linux-kernel - Re: [PATCH net] ice: fix access-beyond-end in the switch code

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:   Wed, 1 Jun 2022 08:26:02 -0700
From:   Tony Nguyen <anthony.l.nguyen@...el.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
CC:     Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>,
        Bruce Allan <bruce.w.allan@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        Wojciech Drewek <wojciech.drewek@...el.com>,
        Marcin Szycik <marcin.szycik@...ux.intel.com>,
        Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@...el.com>,
        <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] ice: fix access-beyond-end in the switch code



On 6/1/2022 3:59 AM, Alexander Lobakin wrote:
> Global `-Warray-bounds` enablement revealed some problems, one of
> which is the way we define and use AQC rules messages.
> In fact, they have a shared header, followed by the actual message,
> which can be of one of several different formats. So it is
> straightforward enough to define that header as a separate struct
> and then embed it into message structures as needed, but currently
> all the formats reside in one union coupled with the header. Then,
> the code allocates only the memory needed for a particular message
> format, leaving the union potentially incomplete.
> There are no actual reads or writes beyond the end of an allocated
> chunk, but at the same time, the whole implementation is fragile and
> backed by an equilibrium rather than strong type and memory checks.
> 
> Define the structures the other way around: one for the common
> header and the rest for the actual formats with the header embedded.
> There are no places where several union members would be used at the
> same time anyway. This allows to use proper struct_size() and let
> the compiler know what is going to be done.
> Finally, unsilence `-Warray-bounds` back for ice_switch.c.
> 
> Other little things worth mentioning:
> * &ice_sw_rule_vsi_list_query is not used anywhere, remove it. It's
>    weird anyway to talk to hardware with purely kernel types
>    (bitmaps);
> * expand the ICE_SW_RULE_*_SIZE() macros to pass a structure
>    variable name to struct_size() to let it do strict typechecking;
> * rename ice_sw_rule_lkup_rx_tx::hdr to ::hdr_data to keep ::hdr
>    for the header structure to have the same name for it constistenly
>    everywhere;
> * drop the duplicate of %ICE_SW_RULE_RX_TX_NO_HDR_SIZE residing in
>    ice_switch.h.
> 
> Fixes: 9daf8208dd4d ("ice: Add support for switch filter programming")
> Fixes: 66486d8943ba ("ice: replace single-element array used for C struct hack")
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@...el.com>
> Reviewed-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
> ---
> To Tony: I'd like this to hit RC1 or RC2, so would it be okay to pass
> through -net directly? Or via some quick pull request would work too
> I guess :)

LGTM. I'm okay with it going to net directly.

Acked-by: Tony Nguyen <anthony.l.nguyen@...el.com>