Re: [PATCH] nfsd: fix memory corruption caused by readdir

[NeilBrown]

On Mon, Mar 04 2019, J. Bruce Fields wrote:

> On Mon, Mar 04, 2019 at 02:08:22PM +1100, NeilBrown wrote:
>> (Note that the commit hash in the Fixes tag is from the 'history'
>>  tree - this bug predates git).
>> Fixes: eb229d253e6c ("[PATCH] kNFSd: fix two xdr-encode bugs for readdirplus reply")
>
> It'd be nice to provide a URL for that.  The one I originally cloned one
> seems to have disappeared.

Fixes-URL: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=eb229d253e6c

Though on reflection, that didn't introduce the bug, it just failed to
fix it properly.  It should be:

Fixes: 0b1d57cf7654 ("[PATCH] kNFSd: Fix nfs3 dentry encoding")
Fixes-URL: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=0b1d57cf7654

>

>> Cc: stable@xxxxxxxxxxxxxxx (v2.6.12+)
>> Signed-off-by: NeilBrown <neilb@xxxxxxxx>
>> ---
>> 
>> Can I still get extra credit for fixing a bug that is 14.5 years old, if
>> I'm the one who introduced it?
>
> Good grief, yes!  Great fix.  Is that a record?
>
> And how did it go undetected so long, and what caused it to surface just
> now?

I suspect two different things need to come together to trigger the bug.
1/ a directory needs to have filename lengths which cause the xdr
   encoding of the readdirplus reply to place the offset across a page
   boundary.
   A typical entry is around 200 bytes, or 50 quads, so there should be
   a 1:50 chance of hitting that, assuming name lengths are evenly
   distributed (which they aren't).
   In the case which triggered the bug, all file names were 43 bytes,
   all filehandles 28 bytes. This means 192 bytes per entry.
   21 entries fit in a page leaving 64 bytes.  This puts the cookie
   on the page boundary.

2/ The *next* entry after the one that crosses the page boundary doesn't
   fit.  In the cases which triggered, the requested size was 0x1110
   (4368).
   That is enough room for 21 entries, but not for 22.

So presumably the client doesn't run Linux - which always asks
for 4096 bytes of directory entry (from a Linux server).
I have no idea what clients the customer was using, but these clients
seem to have a fairly good chance of triggering the bug (when configured
like the customer configured them - maybe).

>
> I once thought about converting this over to the xdr_stream api that
> NFSv4 uses to hide the page-crossing logic now.  But I think it's better
> to leave it alone.

I agree - the code isn't being actively developed, so stability wins
over elegance.


BTW, the readdir (non-plus) code doesn't really need fixing.
nfs3svc_decode_readdirargs() caps the ->count at PAGE_SIZE, so the cookie
can never cross pages. nfs3svc_decode_readdirplusargs() caps it
at max_blocksize.  So if you feel like leaving that part of the change
out, I probably wouldn't complain.

Thanks,
NeilBrown

>
> --b.
>
>> 
>>  fs/nfsd/nfs3proc.c | 16 ++++++++++++++--
>>  fs/nfsd/nfs3xdr.c  |  1 +
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index 9eb8086ea841..c9cf46e0c040 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -463,8 +463,19 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
>>  					&resp->common, nfs3svc_encode_entry);
>>  	memcpy(resp->verf, argp->verf, 8);
>>  	resp->count = resp->buffer - argp->buffer;
>> -	if (resp->offset)
>> -		xdr_encode_hyper(resp->offset, argp->cookie);
>> +	if (resp->offset) {
>> +		loff_t offset = argp->cookie;
>> +
>> +		if (unlikely(resp->offset1)) {
>> +			/* we ended up with offset on a page boundary */
>> +			*resp->offset = htonl(offset >> 32);
>> +			*resp->offset1 = htonl(offset & 0xffffffff);
>> +			resp->offset1 = NULL;
>> +		} else {
>> +			xdr_encode_hyper(resp->offset, offset);
>> +		}
>> +		resp->offset = NULL;
>> +	}
>>  
>>  	RETURN_STATUS(nfserr);
>>  }
>> @@ -533,6 +544,7 @@ nfsd3_proc_readdirplus(struct svc_rqst *rqstp)
>>  		} else {
>>  			xdr_encode_hyper(resp->offset, offset);
>>  		}
>> +		resp->offset = NULL;
>>  	}
>>  
>>  	RETURN_STATUS(nfserr);
>> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
>> index 9b973f4f7d01..83919116d5cb 100644
>> --- a/fs/nfsd/nfs3xdr.c
>> +++ b/fs/nfsd/nfs3xdr.c
>> @@ -921,6 +921,7 @@ encode_entry(struct readdir_cd *ccd, const char *name, int namlen,
>>  		} else {
>>  			xdr_encode_hyper(cd->offset, offset64);
>>  		}
>> +		cd->offset = NULL;
>>  	}
>>  
>>  	/*
>> -- 
>> 2.14.0.rc0.dirty
>>

Attachment: signature.asc
Description: PGP signature