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@xxxxxxxxx> 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
