diff mbox

[07/18] fs_context: fix double free of legacy_fs_context data

Message ID 3014.1531139469@warthog.procyon.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

David Howells July 9, 2018, 12:31 p.m. UTC 
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
---

Comments

Eric Biggers July 10, 2018, 1:17 a.m. UTC | #1 
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 ]---
Eric Biggers July 10, 2018, 1:25 a.m. UTC | #2 
On Mon, Jul 09, 2018 at 06:17:41PM -0700, Eric Biggers wrote:
> 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?

Case in point: if allocating ->fs_private fails here, you'll get a NULL pointer
dereference during put_fs_context() not only from the NULL ->fs_private in
legacy_fs_context_free(), but also from put_filesystem() since ->fs_type hasn't
been set yet.

- Eric
diff mbox

Patch

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;
+	}
+
 	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);
 }
 
 /*
@@ -415,20 +423,28 @@  static void legacy_fs_context_free(struct fs_context *fc)
  */
 static int legacy_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
 {
-	struct legacy_fs_context *ctx = container_of(fc, struct legacy_fs_context, fc);
-	struct legacy_fs_context *src_ctx = container_of(src_fc, struct legacy_fs_context, fc);
+	struct legacy_fs_context *ctx;
+	struct legacy_fs_context *src_ctx = src_fc->fs_private;
+
+	ctx = kmemdup(src_ctx, sizeof(*src_ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
 
 	switch (ctx->param_type) {
 	case LEGACY_FS_MONOLITHIC_PARAMS:
 	case LEGACY_FS_INDIVIDUAL_PARAMS:
 		ctx->legacy_data = kmemdup(src_ctx->legacy_data,
 					   src_ctx->data_size, GFP_KERNEL);
-		if (!ctx->legacy_data)
+		if (!ctx->legacy_data) {
+			kfree(ctx);
 			return -ENOMEM;
+		}
 		/* Fall through */
 	default:
 		break;
 	}
+
+	fc->fs_private = ctx;
 	return 0;
 }
 
@@ -438,7 +454,7 @@  static int legacy_fs_context_dup(struct fs_context *fc, struct fs_context *src_f
  */
 static int legacy_parse_option(struct fs_context *fc, char *opt, size_t len)
 {
-	struct legacy_fs_context *ctx = container_of(fc, struct legacy_fs_context, fc);
+	struct legacy_fs_context *ctx = fc->fs_private;
 	unsigned int size = ctx->data_size;
 
 	if (ctx->param_type != LEGACY_FS_UNSET_PARAMS &&
@@ -471,7 +487,7 @@  static int legacy_parse_option(struct fs_context *fc, char *opt, size_t len)
  */
 static int legacy_parse_monolithic(struct fs_context *fc, void *data, size_t data_size)
 {
-	struct legacy_fs_context *ctx = container_of(fc, struct legacy_fs_context, fc);
+	struct legacy_fs_context *ctx = fc->fs_private;
 
 	if (ctx->param_type != LEGACY_FS_UNSET_PARAMS) {
 		pr_warn("VFS: Can't mix monolithic and individual options\n");
@@ -507,7 +523,7 @@  static int legacy_parse_monolithic(struct fs_context *fc, void *data, size_t dat
  */
 static int legacy_validate(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;
 
 	switch (ctx->param_type) {
 	case LEGACY_FS_UNSET_PARAMS:
@@ -520,7 +536,7 @@  static int legacy_validate(struct fs_context *fc)
 		break;
 	}
 
-	if (ctx->fc.fs_type->fs_flags & FS_BINARY_MOUNTDATA)
+	if (fc->fs_type->fs_flags & FS_BINARY_MOUNTDATA)
 		return 0;
 
 	ctx->secdata = alloc_secdata();
@@ -557,13 +573,13 @@  static int legacy_set_subtype(struct fs_context *fc)
  */
 static int legacy_get_tree(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;
 	struct super_block *sb;
 	struct dentry *root;
 	int ret;
 
-	root = ctx->fc.fs_type->mount(ctx->fc.fs_type, ctx->fc.sb_flags,
-				      ctx->fc.source, ctx->legacy_data,
+	root = fc->fs_type->mount(fc->fs_type, fc->sb_flags,
+				      fc->source, ctx->legacy_data,
 				      ctx->data_size);
 	if (IS_ERR(root))
 		return PTR_ERR(root);
@@ -571,14 +587,14 @@  static int legacy_get_tree(struct fs_context *fc)
 	sb = root->d_sb;
 	BUG_ON(!sb);
 
-	if ((ctx->fc.fs_type->fs_flags & FS_HAS_SUBTYPE) &&
+	if ((fc->fs_type->fs_flags & FS_HAS_SUBTYPE) &&
 	    !fc->subtype) {
 		ret = legacy_set_subtype(fc);
 		if (ret < 0)
 			goto err_sb;
 	}
 
-	ctx->fc.root = root;
+	fc->root = root;
 	return 0;
 
 err_sb: