On Tue, Jan 15, 2008 at 10:31:18AM +1100, Neil Brown wrote:
> On Monday January 14, [email protected] wrote:
> > >
> > > +static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
> > > +{
> > > + __be32 server_ip;
> > > + char *fo_path;
> > > + char *mesg;
> > > +
> > > + /* sanity check */
> > > + if (size <= 0)
> > > + return -EINVAL;
> >
> > Not only is size never negative, it's actually an unsigned type here, so
> > this is a no-op.
>
> No, It it equivalent to
> if (size == 0)
>
> which alternative is clearer and more maintainable is debatable.
>
> >
> > > +
> > > + if (buf[size-1] == '\n')
> > > + buf[size-1] = 0;
> >
> > The other write methods in this file actually just do
> >
> > if (buf[size-1] != '\n')
> > return -EINVAL;
>
> and those which don't check for size == 0 are underflowing an array.
> That should probably be fixed.
?
--b.
commit 6685389d610950126f700d25f3d010c7049441c3
Author: J. Bruce Fields <[email protected]>
Date: Tue Jan 22 17:40:42 2008 -0500
nfsd: more careful input validation in nfsctl write methods
Neil Brown points out that we're checking buf[size-1] in a couple places
without first checking whether size is zero.
Actually, given the implementation of simple_transaction_get(), buf[-1]
is zero, so in both of these cases the subsequent check of the value of
buf[size-1] will catch this case.
But it seems fragile to depend on that, so add explicit checks for this
case.
Signed-off-by: J. Bruce Fields <[email protected]>
Cc: Neil Brown <[email protected]>
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 61015cf..9ed2a2b 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -304,6 +304,9 @@ static ssize_t write_filehandle(struct file *file, char *buf, size_t size)
struct auth_domain *dom;
struct knfsd_fh fh;
+ if (size == 0)
+ return -EINVAL;
+
if (buf[size-1] != '\n')
return -EINVAL;
buf[size-1] = 0;
@@ -663,7 +666,7 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
char *recdir;
int len, status;
- if (size > PATH_MAX || buf[size-1] != '\n')
+ if (size == 0 || size > PATH_MAX || buf[size-1] != '\n')
return -EINVAL;
buf[size-1] = 0;