While running FhGFS-to-NFS stress tests, I noticed this bug (with a
RHEL 6.5 kernel, but I think it also in linux-git).
[ 3428.087489] BUG: unable to handle kernel paging request at ffff880735fb8000
[ 3428.094821] IP: [<ffffffffa038066e>] nfsd4_create+0x27e/0x380 [nfsd]
gdb resolves this to
(gdb) l *(nfsd4_create+0x27e)
0x1469e is in nfsd4_create (fs/nfsd/nfs4proc.c:527).
522 * null-terminate by brute force, since at worst we
523 * will overwrite the first byte of the create namelen
524 * in the XDR buffer, which has already been extracted
525 * during XDR decode.
526 */
527 create->cr_linkname[create->cr_linklen] = 0;
528
529 status = nfsd_symlink(rqstp, &cstate->current_fh,
530 create->cr_name, create->cr_namelen,
531 create->cr_linkname, create->cr_linklen,
create->cr_linkname is set in nfsd4_decode_create and
even current .git does not check for the result of savemem
there.
--
nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference
From: Bernd Schubert <[email protected]>
create->cr_linkname was later used without any check if savemem()
succeeded.
Signed-off-by: Bernd Schubert <[email protected]>
---
fs/nfsd/nfs4xdr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2723c1b..eb65d1e 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -603,6 +603,10 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
READ32(create->cr_linklen);
READ_BUF(create->cr_linklen);
SAVEMEM(create->cr_linkname, create->cr_linklen);
+ status = check_filename(create->cr_linkname,
+ create->cr_linklen);
+ if (status)
+ return status;
break;
case NF4BLK:
case NF4CHR:
> cr_linkname maybe a path contains '/', or '.',
> check_filename will return error nfserr_badname for those path.
>
> IMO, just check whether the length is zero as,
>
> if (create->cr_linklen == 0)
> return nfserr_inval;
>
Hmm, right, checking for a '/' not right here. Checking if link-name is
"." or ".." isn't required, but shouldn't hurt either. But on the other
hand we can leave that to the underlying file system.
Thanks for your review!
Bernd
On 2014/4/19 04:06, Bernd Schubert wrote:
> While running FhGFS-to-NFS stress tests, I noticed this bug (with a
> RHEL 6.5 kernel, but I think it also in linux-git).
>
> [ 3428.087489] BUG: unable to handle kernel paging request at ffff880735fb8000
> [ 3428.094821] IP: [<ffffffffa038066e>] nfsd4_create+0x27e/0x380 [nfsd]
>
> gdb resolves this to
>
> (gdb) l *(nfsd4_create+0x27e)
> 0x1469e is in nfsd4_create (fs/nfsd/nfs4proc.c:527).
> 522 * null-terminate by brute force, since at worst we
> 523 * will overwrite the first byte of the create namelen
> 524 * in the XDR buffer, which has already been extracted
> 525 * during XDR decode.
> 526 */
> 527 create->cr_linkname[create->cr_linklen] = 0;
> 528
> 529 status = nfsd_symlink(rqstp, &cstate->current_fh,
> 530 create->cr_name, create->cr_namelen,
> 531 create->cr_linkname, create->cr_linklen,
>
>
> create->cr_linkname is set in nfsd4_decode_create and
> even current .git does not check for the result of savemem
> there.
>
> --
> nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference
>
> From: Bernd Schubert <[email protected]>
>
> create->cr_linkname was later used without any check if savemem()
> succeeded.
>
>
> Signed-off-by: Bernd Schubert <[email protected]>
> ---
> fs/nfsd/nfs4xdr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2723c1b..eb65d1e 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -603,6 +603,10 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
> READ32(create->cr_linklen);
> READ_BUF(create->cr_linklen);
> SAVEMEM(create->cr_linkname, create->cr_linklen);
> + status = check_filename(create->cr_linkname,
> + create->cr_linklen);
Don't call check_filename() here.
cr_linkname maybe a path contains '/', or '.',
check_filename will return error nfserr_badname for those path.
IMO, just check whether the length is zero as,
if (create->cr_linklen == 0)
return nfserr_inval;
thanks,
Kinglong Mee
On Tue, Apr 22, 2014 at 6:06 PM, Bernd Schubert
<[email protected]> wrote:
>
>> cr_linkname maybe a path contains '/', or '.',
>> check_filename will return error nfserr_badname for those path.
>>
>> IMO, just check whether the length is zero as,
>>
>> if (create->cr_linklen == 0)
>> return nfserr_inval;
>>
> Hmm, right, checking for a '/' not right here. Checking if link-name is
> "." or ".." isn't required, but shouldn't hurt either. But on the other
> hand we can leave that to the underlying file system.
According to rfc3530, 14.2.9, DESCRIPTION
If the newname has a length of 0 (zero), or if newname does not obey
the UTF-8 definition, the error NFS4ERR_INVAL will be returned.
The underlying filesystem will return -ENOENT not -EINVAL.
So, NFSD needs checking it explicitly by-self.
Can you sent the patch of v2?
thanks,
Kinglong Mee