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

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