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

[Kees Cook <keescook@chromium.org>]

On Wed, May 04, 2022 at 08:13:58PM -0700, Jakub Kicinski wrote:
> 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@intel.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?

No, the build warning isn't in the current release, but I'm expecting to
enable the next step of the FORTIFY work in the coming merge window.

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

Yes, due to the enum, but you're right; this could just use min_t(uint...

I'll respin!

-- 
Kees Cook