Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 9 Jul 2018 18:17:41 -0700
From:   Eric Biggers <ebiggers3@gmail.com>
To:     David Howells <dhowells@redhat.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH 07/18] fs_context: fix double free of legacy_fs_context
 data
Message-ID: <20180710011741.GA1014@sol.localdomain>
References: <20180708210154.10423-8-ebiggers3@gmail.com>
 <20180708210154.10423-1-ebiggers3@gmail.com>
 <3014.1531139469@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3014.1531139469@warthog.procyon.org.uk>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jul 09, 2018 at 01:31:09PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > sys_fsmount() calls fc->ops->free() to free the data, zeroes
> > ->fs_private, then proceeds to reuse the context.  But legacy_fs_context
> > doesn't use ->fs_private, so we need to handle zeroing it too; otherwise
> > there's a double free of legacy_fs_context::{legacy_data,secdata}.
> 
> I think the attached is better.  I stopped embedding the fs_context in the
> xxx_fs_context to make certain things simpler, but I missed the legacy
> wrapper.
> 
> David
> ---
> diff --git a/fs/fs_context.c b/fs/fs_context.c
> index f91facc769f7..ab93a0b73dc6 100644
> --- a/fs/fs_context.c
> +++ b/fs/fs_context.c
> @@ -34,7 +34,6 @@ enum legacy_fs_param {
>  };
>  
>  struct legacy_fs_context {
> -	struct fs_context	fc;
>  	char			*legacy_data;	/* Data page for legacy filesystems */
>  	char			*secdata;
>  	size_t			data_size;
> @@ -239,12 +238,21 @@ struct fs_context *vfs_new_fs_context(struct file_system_type *fs_type,
>  				      enum fs_context_purpose purpose)
>  {
>  	struct fs_context *fc;
> -	int ret;
> +	int ret = -ENOMEM;
>  
> -	fc = kzalloc(sizeof(struct legacy_fs_context), GFP_KERNEL);
> +	fc = kzalloc(sizeof(struct fs_context), GFP_KERNEL);
>  	if (!fc)
>  		return ERR_PTR(-ENOMEM);
>  
> +	if (!fs_type->init_fs_context) {
> +		fc->fs_private = kzalloc(sizeof(struct legacy_fs_context),
> +					 GFP_KERNEL);
> +		if (!fc->fs_private)
> +			goto err_fc;
> +
> +		fc->ops = &legacy_fs_context_ops;
> +	}
> +

Why isn't this done in the same place that ->init_fs_context() would otherwise
be called?  It logically does the same thing, right?

>  	fc->purpose	= purpose;
>  	fc->sb_flags	= sb_flags;
>  	fc->fs_type	= get_filesystem(fs_type);
> @@ -277,8 +285,6 @@ struct fs_context *vfs_new_fs_context(struct file_system_type *fs_type,
>  		ret = fc->fs_type->init_fs_context(fc, reference);
>  		if (ret < 0)
>  			goto err_fc;
> -	} else {
> -		fc->ops = &legacy_fs_context_ops;
>  	}
>  
>  	/* Do the security check last because ->init_fs_context may change the
> @@ -395,7 +401,7 @@ EXPORT_SYMBOL(put_fs_context);
>   */
>  static void legacy_fs_context_free(struct fs_context *fc)
>  {
> -	struct legacy_fs_context *ctx = container_of(fc, struct legacy_fs_context, fc);
> +	struct legacy_fs_context *ctx = fc->fs_private;
>  
>  	free_secdata(ctx->secdata);
>  	switch (ctx->param_type) {
> @@ -408,6 +414,8 @@ static void legacy_fs_context_free(struct fs_context *fc)
>  		kfree(ctx->legacy_data);
>  		break;
>  	}
> +
> +	kfree(ctx);
>  }

Okay, but now there's a NULL pointer dereference because fc->ops->free() can be
called with NULL fc->fs_private.  Probably fc->ops->free() shouldn't be called
in that case.

int main()
{
        int fd = syscall(__NR_fsopen, "tmpfs", 0);
        write(fd, "x create", 8);
        syscall(__NR_fsmount, fd, 0, 0);
}

BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
PGD 0 P4D 0 
Oops: 0000 [#1] SMP
CPU: 1 PID: 186 Comm: fsopen Not tainted 4.18.0-rc1-00001-g0f067bdbfeca0 #29
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
RIP: 0010:legacy_fs_context_free+0xc/0x40 fs/fs_context.c:500
Code: 02 75 08 48 c7 42 08 01 00 00 00 31 c0 c3 c7 42 18 01 00 00 00 31 c0 c3 66 0f 1f 44 00 00 55 48 89 e5 53 48 8b 9f 90 00 00 00 <8b> 4b 18 83 f9 04 77 0c b8 01 00 00 00 48 d3 e0 a8 13 75 08 48 8b 
RSP: 0018:ffffc9000079bd88 EFLAGS: 00010282
RAX: ffffffff8118fbe0 RBX: 0000000000000000 RCX: 0000000000000001
RDX: ffff88007c82c0f4 RSI: 0000000000000001 RDI: ffff88007be77700
RBP: ffffc9000079bd90 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff88007c82c000
R13: 0000000000060003 R14: ffff88007d34d020 R15: ffff88007ab8aea8
FS:  00007fee62b79740(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000018 CR3: 0000000001c0f000 CR4: 00000000003406e0
Call Trace:
 put_fs_context+0x4c/0x180 fs/fs_context.c:479
 fscontext_release+0x20/0x30 fs/fsopen.c:196
 __fput+0xbb/0x210 fs/file_table.c:210
 ____fput+0x9/0x10 fs/file_table.c:246
 task_work_run+0x86/0xc0 kernel/task_work.c:113
 exit_task_work include/linux/task_work.h:22 [inline]
 do_exit+0x27a/0xa30 kernel/exit.c:865
 do_group_exit+0x3c/0xc0 kernel/exit.c:968
 __do_sys_exit_group kernel/exit.c:979 [inline]
 __se_sys_exit_group kernel/exit.c:977 [inline]
 __x64_sys_exit_group+0x13/0x20 kernel/exit.c:977
 do_syscall_64+0x4a/0x180 arch/x86/entry/common.c:290
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7fee6224eee8
Code: Bad RIP value.
RSP: 002b:00007ffc3efc0cd8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fee6224eee8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00007fee625386d8 R08: 00000000000000e7 R09: ffffffffffffff80
R10: 00007fee62745100 R11: 0000000000000246 R12: 00007fee625386d8
R13: 00007fee6253dbe0 R14: 0000000000000000 R15: 0000000000000000
CR2: 0000000000000018
---[ end trace 8ac26865cb821d07 ]---