nfsd: fix memory corruption caused by readdir
diff mbox series

Message ID 87lg1vs5eh.fsf@notabene.neil.brown.name
State Accepted
Commit b602345da6cbb135ba68cf042df8ec9a73da7981
Headers show
Series
  • nfsd: fix memory corruption caused by readdir
Related show

Commit Message

NeilBrown March 4, 2019, 3:08 a.m. UTC
If the result of an NFSv3 readdir{,plus} request results in the
"offset" on one entry having to be split across 2 pages, and is sized
so that the next directory entry doesn't fix in the requested size,
then memory corruption can happen.

When encode_entry() is called after encoding the last entry that fits,
it notices that ->offset and ->offset1 are set, and so stores the
offset value in the two pages as required.  It clears ->offset1 but
*does not* clear ->offset.

Normally this omission doesn't matter as encode_entry_baggage() will
be called, and will set ->offset to a suitable value (not on a page
boundary).
But in the case where cd->buflen < elen and nfserr_toosmall is
returned, ->offset is not reset.

This means that nfsd3proc_readdirplus will see ->offset with a value 4
bytes before the end of a page, and ->offset1 set to NULL.
It will try to write 8bytes to ->offset.
If we are lucky, the next page will be read-only, and the system will
  BUG: unable to handle kernel paging request at...

If we are unlucky, some innocent page will have the first 4 bytes
corrupted.

nfsd3proc_readdir() doesn't even check for ->offset1, it just blindly
writes 8 bytes to the offset wherever it is.

Fix this by clearing ->offset after it is used, and copying the
->offset handling code from nfsd3_proc_readdirplus into
nfsd3_proc_readdir.

(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")
Cc: stable@vger.kernel.org (v2.6.12+)
Signed-off-by: NeilBrown <neilb@suse.com>
---

Can I still get extra credit for fixing a bug that is 14.5 years old, if
I'm the one who introduced it?

 fs/nfsd/nfs3proc.c | 16 ++++++++++++++--
 fs/nfsd/nfs3xdr.c  |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

J. Bruce Fields March 4, 2019, 4:47 p.m. UTC | #1
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.

> Cc: stable@vger.kernel.org (v2.6.12+)
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> 
> 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 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.

--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
>
NeilBrown March 4, 2019, 11:48 p.m. UTC | #2
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@vger.kernel.org (v2.6.12+)
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> 
>> 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
>>
J. Bruce Fields March 5, 2019, 9:42 p.m. UTC | #3
On Tue, Mar 05, 2019 at 10:48:45AM +1100, NeilBrown wrote:
> 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

Oh, so we can blame Olaf.  Even better.

> > 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).

Thanks for the explanation!

> > 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.

Eh, makes sense to me to fix it.

--b.

Patch
diff mbox series

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;
 	}
 
 	/*