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

[Jakub Kicinski]

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@xxxxxxxxx>
> Link: https://lore.kernel.org/lkml/202205031926.FVP7epJM-lkp@xxxxxxxxx
> 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@xxxxxxxxxxx>
> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: Paolo Abeni <pabeni@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  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);