Hi Bruce-
On Jan 28, 2008, at 3:15 PM, J. Bruce Fields wrote:
On Mon, Jan 28, 2008 at 01:00:47PM -0500, Chuck Lever wrote:
On Jan 25, 2008, at 6:15 PM, J. Bruce Fields wrote:
The length "nbytes" passed into read_buf should never be
negative, but
we check only for too-large values of "nbytes", not for too-small
values. Make nbytes unsigned, so it's clear that the former
tests are
sufficient. (Despite this read_buf() currently correctly returns an
xdr
error in the case of a negative length, thanks to an unsigned
comparison with size_of() and bounds-checking in kmalloc(). This
seems
very fragile, though.)
Signed-off-by: J. Bruce Fields <[EMAIL PROTECTED]>
---
fs/nfsd/nfs4xdr.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 5733394..25c7ae2 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -148,7 +148,7 @@ xdr_error: \
} \
} while (0)
-static __be32 *read_buf(struct nfsd4_compoundargs *argp, int
nbytes)
+static __be32 *read_buf(struct nfsd4_compoundargs *argp, u32
nbytes)
{
/* We want more bytes than seem to be available.
* Maybe we need a new page, maybe we have just run out
Changing nbytes to an unsigned introduces a mixed-sign comparison:
int avail = (char*)argp->end - (char*)argp->p;
__be32 *p;
if (avail + argp->pagelen < nbytes) <<<<<<
"avail" and "argp->pagelen" are signed ints.
OK. Is the result incorrect?
Whether it is or it isn't, I'm not comfortable with introducing a
mixed-sign comparison for no reason. It's not a correctness issue,
it's a maintainability issue.
(Any objections to just making "avail" unsigned?)
If we can guarantee that argp->end is always larger than argp->p...
then there's no need for "avail" to be a signed int. However:
return NULL;
if (avail + PAGE_SIZE < nbytes) /* need more than a
page !! */
return NULL;
PAGE_SIZE is unsigned long (defined as "1UL << PAGE_SHIFT"), so
changing
nbytes may have additional unintended consequences.
...and since that wasn't noticed or addressed by this patch, it
suggests that there may be other issues that the patch author didn't
address and that reviewers didn't catch.
We need careful review of everywhere argp->pagelen is used throughout
this file. But I think just this simple fix is inadequate.
Sure, such a review would be welcomed.
The patch still seems an obvious improvement to me (and you don't seem
to be arguing the contrary?) so I'm inclined to apply it pending that
further work.
I agree that nbytes should be unsigned, but I'm arguing that you will
need more than just changing nbytes to a u32 to produce a truly clean
fix; therefore, I'm suggesting you should hold off on this until we
have a complete fix that is a result of a thorough review of that code.
My experience with the NFSv4 XDR routines is that there are scores of
these little nuisances waiting to bite us (and a brief review of how
argp->pagelen is used in nfs4xdr.c vindicates this experience).
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
