| Message ID | ed14a180-56c8-40f2-acf7-26a787eb3769@suswa.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] nfsd: prevent integer overflow in decode_cb_compound4res() | expand |
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>
> 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
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
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;
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(+)