Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3847 bytes --]

On Mon, Apr 24 2017, J. Bruce Fields wrote:

> And, another problem spotted by the Synposys folks.
>
> I'll give these some more testing and hope to send a pull request in
> another day or two.
>
> --b.
>
> commit a0aa2db91590
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Fri Apr 21 15:26:30 2017 -0400
>
>     nfsd: stricter decoding of write-like NFSv2/v3 ops
>     
>     The NFSv2/v3 code does not systematically check whether we decode past
>     the end of the buffer.  This generally appears to be harmless, but there
>     are a few places where we do arithmetic on the pointers involved and
>     don't account for the possibility that a length could be negative.  Add
>     checks to catch these.
>     
>     Reported-by: Tuomas Haanpää <thaan@synopsys.com>
>     Reported-by: Ari Kauppi <ari@synopsys.com>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>

The code looks right ... but ... 

>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index dba2ff8eaa68..41cc47bf9d00 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -358,6 +358,7 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  {
>  	unsigned int len, v, hdr, dlen;
>  	u32 max_blocksize = svc_max_payload(rqstp);
> +	struct kvec *head = rqstp->rq_arg.head;
>  
>  	p = decode_fh(p, &args->fh);
>  	if (!p)
> @@ -367,6 +368,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	args->count = ntohl(*p++);
>  	args->stable = ntohl(*p++);
>  	len = args->len = ntohl(*p++);
> +	if ((void *)p > head->iov_base + head->iov_len)
> +		return 0;

I'm in two minds here.
On the one hand, the change for NFSv2 could be used here unchanged,
which would make the change slightly smaller and easier to review as
two parts would be identical.

On the other hand I think there is value in defining the "head" local
variable, but to fully realize that value you would need to define
"tail" as well, and then change

	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
		+ rqstp->rq_arg.tail[0].iov_len - hdr;

to

        dlen = head->iov_len + rqstp->rq_arg.page_len + tail->iov_len - hdr;

i.e. either keep it simple (like the v2 code) or make it tidy (with head
and tail), but not half-and-half??


>  	/*
>  	 * The count must equal the amount of data passed.
>  	 */
> @@ -466,11 +469,15 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p,
>  	len = ntohl(*p++);
>  	if (len == 0 || len > NFS3_MAXPATHLEN || len >= PAGE_SIZE)
>  		return 0;
> +	if (!*(rqstp->rq_next_page))
> +		return 0;

Why the extra parentheses?  "->" is at the highest precedence level for C.

>  	args->tname = new = page_address(*(rqstp->rq_next_page++));
>  	args->tlen = len;
>  	/* first copy and check from the first page */
>  	old = (char*)p;
>  	vec = &rqstp->rq_arg.head[0];
> +	if ((void *)old > vec->iov_base + vec->iov_len)
> +		return 0;
>  	avail = vec->iov_len - (old - (char*)vec->iov_base);

We seem to be repeating a calculation here.
I would prefer to make 'avail' a signed int then add

        if (avail < 0)
              return 0;

That would seem more "obviously correct".

But the code is correct as it stands.
Reviewed-by: NeilBrown <neilb@suse.com>

Thanks,
NeilBrown


>  	while (len && avail && *old) {
>  		*new++ = *old++;
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 41b468a6a90f..7a0eed7c619d 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -301,6 +301,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	 * bytes.
>  	 */
>  	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
> +	if (hdr > rqstp->rq_arg.head[0].iov_len)
> +		return 0;
>  	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
>  		- hdr;
>  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]