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 <dan.carpenter@xxxxxxxxxx>
> ---
> 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
