linux-hardening - Re: [PATCH] net: chelsio: cxgb4: Avoid potential negative array offset

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 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]

Date:   Wed, 4 May 2022 20:13:58 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Raju Rangoju <rajur@...lsio.com>,
        kernel test robot <lkp@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        stable@...r.kernel.org, Heiner Kallweit <hkallweit1@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] net: chelsio: cxgb4: Avoid potential negative array
 offset

On Tue,  3 May 2022 07:44:25 -0700 Kees Cook wrote:
> Using min_t(int, ...) as a potential array index implies to the compiler
> that negative offsets should be allowed. This is not the case, though.
> Replace min_t() with clamp_t(). Fixes the following warning exposed
> under future CONFIG_FORTIFY_SOURCE improvements:

> Additionally remove needless cast from u8[] to char * in last strim()
> call.
> 
> Reported-by: kernel test robot <lkp@...el.com>
> Link: https://lore.kernel.org/lkml/202205031926.FVP7epJM-lkp@intel.com
> Fixes: fc9279298e3a ("cxgb4: Search VPD with pci_vpd_find_ro_info_keyword()")
> Fixes: 24c521f81c30 ("cxgb4: Use pci_vpd_find_id_string() to find VPD ID string")

Is it needed in the current release?

> Cc: Raju Rangoju <rajur@...lsio.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: netdev@...r.kernel.org
> Cc: stable@...r.kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
> index e7b4e3ed056c..f119ec7323e5 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
> @@ -2793,14 +2793,14 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>  		goto out;
>  	na = ret;
>  
> -	memcpy(p->id, vpd + id, min_t(int, id_len, ID_LEN));
> +	memcpy(p->id, vpd + id, clamp_t(int, id_len, 0, ID_LEN));

The typing is needed because of the enum, right? The variable is
unsigned, seems a little strange to use clamp(int, ..., 0, constant)
min(unsigned int, ..., constant) will be equivalent with fewer branches.
Is it just me?

>  	strim(p->id);
> -	memcpy(p->sn, vpd + sn, min_t(int, sn_len, SERNUM_LEN));
> +	memcpy(p->sn, vpd + sn, clamp_t(int, sn_len, 0, SERNUM_LEN));
>  	strim(p->sn);
> -	memcpy(p->pn, vpd + pn, min_t(int, pn_len, PN_LEN));
> +	memcpy(p->pn, vpd + pn, clamp_t(int, pn_len, 0, PN_LEN));
>  	strim(p->pn);
> -	memcpy(p->na, vpd + na, min_t(int, na_len, MACADDR_LEN));
> -	strim((char *)p->na);
> +	memcpy(p->na, vpd + na, clamp_t(int, na_len, 0, MACADDR_LEN));
> +	strim(p->na);
>  
>  out:
>  	vfree(vpd);