[PATCH] nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

[PATCH] nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

[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 <bernd.schubert@itwm.fraunhofer.de>

create->cr_linkname was later used without any check if savemem()
succeeded.


Signed-off-by: Bernd Schubert <bernd.schubert@fastmail.fm>
---
 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:

Re: [PATCH] nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

[Kinglong Mee]

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 <bernd.schubert@itwm.fraunhofer.de>
>
> create->cr_linkname was later used without any check if savemem()
> succeeded.
>
>
> Signed-off-by: Bernd Schubert <bernd.schubert@fastmail.fm>
> ---
>   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

Re: [PATCH] nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

[Bernd Schubert]

> 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

Re: [PATCH] nfsd: nfsd4_decode_create: Fix a possible NULL pointer dereference

[Kinglong Mee]

On Tue, Apr 22, 2014 at 6:06 PM, Bernd Schubert
<bernd.schubert@fastmail.fm> 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