These lengths come from xdr_stream_decode_u32() and so we should be a
bit careful with them. Use size_add() and struct_size() to avoid
integer overflows. Saving size_add()/struct_size() results to a u32 is
unsafe because it truncates away the high bits.
Also generally storing sizes in longs is safer. Most systems these days
use 64 bit CPUs. It's harder for an addition to overflow 64 bits than
it is to overflow 32 bits. Also functions like vmalloc() can
successfully allocate UINT_MAX bytes, but nothing can allocate ULONG_MAX
bytes.
Signed-off-by: Dan Carpenter <[email protected]>
---
I think my patch 1 fixes any real issues. It's hard to assign a Fixes
tag to this.
fs/nfsd/nfs4xdr.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c7bfd2180e3f..42b41d55d4ed 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -118,11 +118,11 @@ static int zero_clientid(clientid_t *clid)
* operation described in @argp finishes.
*/
static void *
-svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
+svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, size_t len)
{
struct svcxdr_tmpbuf *tb;
- tb = kmalloc(sizeof(*tb) + len, GFP_KERNEL);
+ tb = kmalloc(struct_size(tb, buf, len), GFP_KERNEL);
if (!tb)
return NULL;
tb->next = argp->to_free;
@@ -138,9 +138,9 @@ svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
* buffer might end on a page boundary.
*/
static char *
-svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
+svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, size_t len)
{
- char *p = svcxdr_tmpalloc(argp, len + 1);
+ char *p = svcxdr_tmpalloc(argp, size_add(len, 1));
if (!p)
return NULL;
@@ -150,7 +150,7 @@ svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
}
static void *
-svcxdr_savemem(struct nfsd4_compoundargs *argp, __be32 *p, u32 len)
+svcxdr_savemem(struct nfsd4_compoundargs *argp, __be32 *p, size_t len)
{
__be32 *tmp;
@@ -2146,7 +2146,7 @@ nfsd4_decode_clone(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
*/
static __be32
nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
- char **bufp, u32 buflen)
+ char **bufp, size_t buflen)
{
struct page **pages = xdr->pages;
struct kvec *head = xdr->head;
--
2.43.0
On Thu, May 09, 2024 at 01:48:28PM +0300, Dan Carpenter wrote:
> These lengths come from xdr_stream_decode_u32() and so we should be a
> bit careful with them. Use size_add() and struct_size() to avoid
> integer overflows. Saving size_add()/struct_size() results to a u32 is
> unsafe because it truncates away the high bits.
>
> Also generally storing sizes in longs is safer. Most systems these days
> use 64 bit CPUs. It's harder for an addition to overflow 64 bits than
> it is to overflow 32 bits. Also functions like vmalloc() can
> successfully allocate UINT_MAX bytes, but nothing can allocate ULONG_MAX
> bytes.
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> I think my patch 1 fixes any real issues. It's hard to assign a Fixes
> tag to this.
I agree that this is a defensive change only. As it is late in the
cycle and this doesn't seem urgent, I would prefer to queue this
change for v6.11.
> fs/nfsd/nfs4xdr.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c7bfd2180e3f..42b41d55d4ed 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -118,11 +118,11 @@ static int zero_clientid(clientid_t *clid)
> * operation described in @argp finishes.
> */
> static void *
> -svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
> +svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, size_t len)
> {
> struct svcxdr_tmpbuf *tb;
>
> - tb = kmalloc(sizeof(*tb) + len, GFP_KERNEL);
> + tb = kmalloc(struct_size(tb, buf, len), GFP_KERNEL);
> if (!tb)
> return NULL;
> tb->next = argp->to_free;
> @@ -138,9 +138,9 @@ svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
> * buffer might end on a page boundary.
> */
> static char *
> -svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
> +svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, size_t len)
> {
> - char *p = svcxdr_tmpalloc(argp, len + 1);
> + char *p = svcxdr_tmpalloc(argp, size_add(len, 1));
>
> if (!p)
> return NULL;
> @@ -150,7 +150,7 @@ svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
> }
>
> static void *
> -svcxdr_savemem(struct nfsd4_compoundargs *argp, __be32 *p, u32 len)
> +svcxdr_savemem(struct nfsd4_compoundargs *argp, __be32 *p, size_t len)
> {
> __be32 *tmp;
>
> @@ -2146,7 +2146,7 @@ nfsd4_decode_clone(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
> */
> static __be32
> nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
> - char **bufp, u32 buflen)
> + char **bufp, size_t buflen)
> {
> struct page **pages = xdr->pages;
> struct kvec *head = xdr->head;
> --
> 2.43.0
>
--
Chuck Lever
On Thu, May 09, 2024 at 09:19:48AM -0400, Chuck Lever wrote:
> On Thu, May 09, 2024 at 01:48:28PM +0300, Dan Carpenter wrote:
> > These lengths come from xdr_stream_decode_u32() and so we should be a
> > bit careful with them. Use size_add() and struct_size() to avoid
> > integer overflows. Saving size_add()/struct_size() results to a u32 is
> > unsafe because it truncates away the high bits.
> >
> > Also generally storing sizes in longs is safer. Most systems these days
> > use 64 bit CPUs. It's harder for an addition to overflow 64 bits than
> > it is to overflow 32 bits. Also functions like vmalloc() can
> > successfully allocate UINT_MAX bytes, but nothing can allocate ULONG_MAX
> > bytes.
> >
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > I think my patch 1 fixes any real issues. It's hard to assign a Fixes
> > tag to this.
>
> I agree that this is a defensive change only. As it is late in the
> cycle and this doesn't seem urgent, I would prefer to queue this
> change for v6.11.
>
Sounds good. I would imagine that eventually it will make its way back
to the stable kernels but it's not a rush.
regards,
dan carpenter