linux-kernel - Re: [PATCH 2/2] NFSD: harden svcxdr_dupstr() and svcxdr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZjzNdLynC7WxwLno@tissot.1015granger.net>
Date: Thu, 9 May 2024 09:19:48 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <kolga@...app.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>, linux-nfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/2] NFSD: harden svcxdr_dupstr() and svcxdr_tmpalloc()
 against integer overflows

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@...aro.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