Re: [PATCH 2/2] NFSD: harden svcxdr_dupstr() and svcxdr_tmpalloc() against integer overflows

[Chuck Lever <chuck.lever@oracle.com>]

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@linaro.org>
> ---
> 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