linux-kernel - Re: [PATCH 07/18] fs_context: fix double free of legacy_fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Jul 2018 18:25:18 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     David Howells <dhowells@...hat.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH 07/18] fs_context: fix double free of legacy_fs_context
 data

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@...il.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