Skip to contentSkip to conversation

nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

PATCH In review Bernd Schubert 2 participants 4 messages Apr 18, 2014 – May 24, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Bernd Schubert

author
#0 lore raw

PATCH 1/1 nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

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(+)
fs/nfsd/nfs4xdr.c +4 -0
index 2723c1b..eb65d1e 100644
@@ -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:
#1 lore raw
Quoted 45 lines · Bernd Schubert

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(+)
fs/nfsd/nfs4xdr.c +0 -0
index 2723c1b..eb65d1e 100644

fs/nfsd/nfs4xdr.c nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create · line 607

Quoted 5 lines
fs/nfsd/nfs4xdr.c +2 -0
@@ -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

author
#2 lore raw
Quoted 9 lines

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

#3 lore raw
Quoted 4 lines · Bernd Schubert
Quoted 4 lines

cr_linkname maybe a path contains '/', or '.', check_filename will return error nfserr_badname for those path.

Quoted 8 lines
Quoted 5 lines

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