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:25:18 -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: <20180710012518.GB1014@sol.localdomain>
References: <20180708210154.10423-8-ebiggers3@gmail.com>
 <20180708210154.10423-1-ebiggers3@gmail.com>
 <3014.1531139469@warthog.procyon.org.uk>
 <20180710011741.GA1014@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180710011741.GA1014@sol.localdomain>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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