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: <202205050919.496AC46@keescook>
Date:   Thu, 5 May 2022 09:23:23 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Jakub Kicinski <kuba@...nel.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 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@...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?

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

Powered by blists - more mailing lists