[v2] nfsd: prevent integer overflow in decode_cb_compound4res()

[PATCH v2] nfsd: prevent integer overflow in decode_cb_compound4res()

Posted by Dan Carpenter 2 months, 1 week ago

If "length" is >= U32_MAX - 3 then the "length + 4" addition can result
in an integer overflow.  The impact of this bug is not totally clear to
me, but it's safer to not allow the integer overflow.

Check that "length" is valid right away before doing any math.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: Check that "len" is valid instead of just checking for integer
    overflows.

 fs/nfsd/nfs4callback.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 43b8320c8255..0f5b7b6fba74 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -317,6 +317,8 @@ static int decode_cb_compound4res(struct xdr_stream *xdr,
 	hdr->status = be32_to_cpup(p++);
 	/* Ignore the tag */
 	length = be32_to_cpup(p++);
+	if (unlikely(length > xdr->buf->len))
+		goto out_overflow;
 	p = xdr_inline_decode(xdr, length + 4);
 	if (unlikely(p == NULL))
 		goto out_overflow;
-- 
2.34.1

Re: [PATCH v2] nfsd: prevent integer overflow in decode_cb_compound4res()

Posted by Chuck Lever III 2 months, 1 week ago


> On Sep 19, 2024, at 1:12 AM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> If "length" is >= U32_MAX - 3 then the "length + 4" addition can result
> in an integer overflow.  The impact of this bug is not totally clear to
> me, but it's safer to not allow the integer overflow.
> 
> Check that "length" is valid right away before doing any math.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> v2: Check that "len" is valid instead of just checking for integer
>    overflows.
> 
> fs/nfsd/nfs4callback.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 43b8320c8255..0f5b7b6fba74 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -317,6 +317,8 @@ static int decode_cb_compound4res(struct xdr_stream *xdr,
> hdr->status = be32_to_cpup(p++);
> /* Ignore the tag */
> length = be32_to_cpup(p++);
> + if (unlikely(length > xdr->buf->len))
> + goto out_overflow;
> p = xdr_inline_decode(xdr, length + 4);
> if (unlikely(p == NULL))
> goto out_overflow;
> -- 
> 2.34.1
> 

Hi Dan, I've already gone with

https://lore.kernel.org/linux-nfs/172658972371.2454.15715383792386404543.stgit@oracle-102.chuck.lever.oracle.com.nfsv4.dev/T/#u

Let me know if that doesn't make sense.

--
Chuck Lever

Re: [PATCH v2] nfsd: prevent integer overflow in decode_cb_compound4res()

Posted by Dan Carpenter 2 months, 1 week ago

On Thu, Sep 19, 2024 at 02:02:19PM +0000, Chuck Lever III wrote:
> 
> 
> > On Sep 19, 2024, at 1:12 AM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > 
> > If "length" is >= U32_MAX - 3 then the "length + 4" addition can result
> > in an integer overflow.  The impact of this bug is not totally clear to
> > me, but it's safer to not allow the integer overflow.
> > 
> > Check that "length" is valid right away before doing any math.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > v2: Check that "len" is valid instead of just checking for integer
> >    overflows.
> > 
> > fs/nfsd/nfs4callback.c | 2 ++
> > 1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 43b8320c8255..0f5b7b6fba74 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -317,6 +317,8 @@ static int decode_cb_compound4res(struct xdr_stream *xdr,
> > hdr->status = be32_to_cpup(p++);
> > /* Ignore the tag */
> > length = be32_to_cpup(p++);
> > + if (unlikely(length > xdr->buf->len))
> > + goto out_overflow;
> > p = xdr_inline_decode(xdr, length + 4);
> > if (unlikely(p == NULL))
> > goto out_overflow;
> > -- 
> > 2.34.1
> > 
> 
> Hi Dan, I've already gone with
> 
> https://lore.kernel.org/linux-nfs/172658972371.2454.15715383792386404543.stgit@oracle-102.chuck.lever.oracle.com.nfsv4.dev/T/#u
> 
> Let me know if that doesn't make sense.

Oh, sorry, I got mixed up which things were patched.  That looks good.
The truth is I always prefer when people give me a reported by tag so I
don't have to write a v2 patch.  It just saves a back and forth.
Thanks!

regards,
dan carpenter

Re: [PATCH v2] nfsd: prevent integer overflow in decode_cb_compound4res()

Posted by Jeff Layton 2 months, 1 week ago

On Thu, 2024-09-19 at 11:12 +0300, Dan Carpenter wrote:
> If "length" is >= U32_MAX - 3 then the "length + 4" addition can result
> in an integer overflow.  The impact of this bug is not totally clear to
> me, but it's safer to not allow the integer overflow.
> 
> Check that "length" is valid right away before doing any math.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> v2: Check that "len" is valid instead of just checking for integer
>     overflows.
> 
>  fs/nfsd/nfs4callback.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 43b8320c8255..0f5b7b6fba74 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -317,6 +317,8 @@ static int decode_cb_compound4res(struct xdr_stream *xdr,
>  	hdr->status = be32_to_cpup(p++);
>  	/* Ignore the tag */
>  	length = be32_to_cpup(p++);
> +	if (unlikely(length > xdr->buf->len))
> +		goto out_overflow;
>  	p = xdr_inline_decode(xdr, length + 4);
>  	if (unlikely(p == NULL))
>  		goto out_overflow;

Reviewed-by: Jeff Layton <jlayton@kernel.org>