| Message ID | 87lg1vs5eh.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: fix memory corruption caused by readdir | expand |
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 >
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 >>
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.
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; } /*
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(-)