Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1529de75f7a35e7847c292705c936cdb4649be39.camel@kernel.org>
Subject: Re: [PATCH] SUNRPC: clean up integer overflow check
From:   Jeff Layton <jlayton@kernel.org>
To:     Dan Carpenter <dan.carpenter@linaro.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Dmitry Antipov <dmantipov@yandex.ru>
Cc:     Anna Schumaker <anna@kernel.org>,
        Chuck Lever <chuck.lever@oracle.com>,
        Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-nfs@vger.kernel.org,
        llvm@lists.linux.dev, kernel-janitors@vger.kernel.org
Date:   Fri, 30 Jun 2023 08:36:59 -0400
In-Reply-To: <2390fdc8-13fa-4456-ab67-44f0744db412@moroto.mountain>
References: <2390fdc8-13fa-4456-ab67-44f0744db412@moroto.mountain>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-06-30 at 12:46 +0300, Dan Carpenter wrote:
> This integer overflow check works as intended but Clang and GCC and warn
> about it when compiling with W=3D1.
>=20
>     include/linux/sunrpc/xdr.h:539:17: error: comparison is always false
>     due to limited range of data type [-Werror=3Dtype-limits]
>=20
> Use size_mul() to prevent the integer overflow.  It silences the warning
> and it's cleaner as well.
>=20
> Reported-by: Dmitry Antipov <dmantipov@yandex.ru>
> Closes: https://lore.kernel.org/all/20230601143332.255312-1-dmantipov@yan=
dex.ru/
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Btw, since the Clang developers are automatically CC'd, here is how I
> silenced this type of false positive in Smatch:
>=20
> 1) Check that longs are 64 bit.
> 2) Check that the right hand side has a SIZE_MAX.  SIZE_MAX is defined
>    as -1UL so you want both the type and the value to match.
> 3) Then on the other the other side, check that the type is uint.
>=20
> I'm looking at this code now in Smatch and it's kind of ugly, and also
> there are some other places where I need to apply the same logic...
>=20
>  include/linux/sunrpc/xdr.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>=20
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index f89ec4b5ea16..dbf7620a2853 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -775,9 +775,7 @@ xdr_stream_decode_uint32_array(struct xdr_stream *xdr=
,
> =20
>  	if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0))
>  		return -EBADMSG;
> -	if (len > SIZE_MAX / sizeof(*p))
> -		return -EBADMSG;
> -	p =3D xdr_inline_decode(xdr, len * sizeof(*p));
> +	p =3D xdr_inline_decode(xdr, size_mul(len, sizeof(*p)));
>  	if (unlikely(!p))
>  		return -EBADMSG;
>  	if (array =3D=3D NULL)


Acked-by: Jeff Layton <jlayton@kernel.org>