Re: [PATCH] NFSD: prevent integer overflow on 32 bit systems

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2022-03-14 at 18:05 +0000, Chuck Lever III wrote:
> 
> 
> > On Mar 14, 2022, at 1:03 PM, Dan Carpenter
> > <dan.carpenter@oracle.com> wrote:
> > 
> > On Mon, Mar 14, 2022 at 05:45:59PM +0300, Chuck Lever III wrote:
> > > Hi Dan-
> > > 
> > > > On Mar 14, 2022, at 10:09 AM, Dan Carpenter
> > > > <dan.carpenter@oracle.com> wrote:
> > > > 
> > > > On a 32 bit system, the "len * sizeof(*p)" operation can have
> > > > an
> > > > integer overflow.
> > > > 
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > ---
> > > > It's hard to pick a Fixes tag for this...  The temptation is to
> > > > say:
> > > > Fixes: 37c88763def8 ("NFSv4; Clean up XDR encoding of type
> > > > bitmap4")
> > > > But there were integer overflows in the code before that as
> > > > well.
> > > > 
> > > > include/linux/sunrpc/xdr.h | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/include/linux/sunrpc/xdr.h
> > > > b/include/linux/sunrpc/xdr.h
> > > > index b519609af1d0..61b92e6b9813 100644
> > > > --- a/include/linux/sunrpc/xdr.h
> > > > +++ b/include/linux/sunrpc/xdr.h
> > > > @@ -731,6 +731,8 @@ xdr_stream_decode_uint32_array(struct
> > > > xdr_stream *xdr,
> > > > 
> > > >         if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0))
> > > >                 return -EBADMSG;
> > > > +       if (len > ULONG_MAX / sizeof(*p))
> > > > +               return -EBADMSG;
> > > 
> > > IIUC xdr_inline_decode() returns NULL if the value of
> > > "len * sizeof(p)" is larger than the remaining XDR buffer
> > > size. I don't believe this extra check is necessary.
> > > 
> > 
> > Yes, but because of the integer overflow then "len * sizeof(*p))"
> > will
> > be a very reasonable small number.
> 
> Got it.

Shouldn't we technically rather specify SIZE_MAX instead of ULONG_MAX
since xdr_inline_decode() takes a size_t?


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com