Re: [PATCH 1/3] nfsd: fix rare symlink decoding bug

["J. Bruce Fields" <bfields@redhat.com>]

On Mon, Jun 23, 2014 at 07:28:16PM -0400, Trond Myklebust wrote:
> On Mon, Jun 23, 2014 at 6:10 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 2d305a1..56bdf4a 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -600,7 +600,18 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
> >                 READ_BUF(4);
> >                 create->cr_linklen = be32_to_cpup(p++);
> >                 READ_BUF(create->cr_linklen);
> > -               SAVEMEM(create->cr_linkname, create->cr_linklen);
> > +               /*
> > +                * The VFS will want a null-terminated string, and
> > +                * null-terminating in place isn't safe since this might
> > +                * end on a page boundary:
> > +                */
> > +               create->cr_linkname =
> > +                               kmalloc(create->cr_linklen + 1, GFP_KERNEL);
> > +               if (!create->cr_linkname)
> > +                       return nfserr_jukebox;
> > +               memcpy(create->cr_linkname, p, create->cr_linklen);
> > +               create->cr_linkname[create->cr_linklen] = '\0';
> > +               defer_free(argp, kfree, create->cr_linkname);
> >                 break;
> >         case NF4BLK:
> >         case NF4CHR:
> 
> Note that "defer_free()" does yet another allocation here in order to
> make space for a small 'struct tmpbuf' structure. Unlike the first
> allocation, there is no check for whether or not that second
> allocation is successful above, so you can easily end up with a silent
> memory leakage (ditto for a number of other callers of defer_free()).
> 
> Looking around at all the other users, wouldn't it perhaps make sense
> to replace defer_free() with a helper that does just a single
> allocation for both the memory buffer and the struct tmpbuf?

Yeah, thanks, working on it....

--b.