linux-hardening - Re: [Intel-wired-lan] [PATCH][next] ixgbe: Fix out-of-bounds warning in ixgbe_host_interface

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: <CAG48ez2jr_8MbY_sNXfwvs7WsF-5f9j=U4-66dTcgXd2msr39A@mail.gmail.com>
Date:   Wed, 17 Mar 2021 19:57:44 +0100
From:   Jann Horn <jannh@...gle.com>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        intel-wired-lan@...ts.osuosl.org, linux-hardening@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH][next] ixgbe: Fix out-of-bounds warning
 in ixgbe_host_interface_command()

On Wed, Mar 17, 2021 at 7:27 PM Gustavo A. R. Silva
<gustavo@...eddedor.com> wrote:
> On 3/17/21 12:11, Jann Horn wrote:
> > On Wed, Mar 17, 2021 at 8:43 AM Gustavo A. R. Silva
> > <gustavoars@...nel.org> wrote:
> >> Fix the following out-of-bounds warning by replacing the one-element
> >> array in an anonymous union with a pointer:
> >>
> >>   CC [M]  drivers/net/ethernet/intel/ixgbe/ixgbe_common.o
> >> drivers/net/ethernet/intel/ixgbe/ixgbe_common.c: In function ‘ixgbe_host_interface_command’:
> >> drivers/net/ethernet/intel/ixgbe/ixgbe_common.c:3729:13: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> >>  3729 |   bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> >>       |   ~~~~~~~~~~^~~~
> >> drivers/net/ethernet/intel/ixgbe/ixgbe_common.c:3682:7: note: while referencing ‘u32arr’
> >>  3682 |   u32 u32arr[1];
> >>       |       ^~~~~~
> >>
> >> This helps with the ongoing efforts to globally enable -Warray-bounds.
> >>
> >> Notice that, the usual approach to fix these sorts of issues is to
> >> replace the one-element array with a flexible-array member. However,
> >> flexible arrays should not be used in unions. That, together with the
> >> fact that the array notation is not being affected in any ways, is why
> >> the pointer approach was chosen in this case.
> >>
> >> Link: https://github.com/KSPP/linux/issues/109
> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> >> ---
> >>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> >> index 62ddb452f862..bff3dc1af702 100644
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> >> @@ -3679,7 +3679,7 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, void *buffer,
> >>         u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
> >>         union {
> >>                 struct ixgbe_hic_hdr hdr;
> >> -               u32 u32arr[1];
> >> +               u32 *u32arr;
> >>         } *bp = buffer;
> >>         u16 buf_len, dword_len;
> >>         s32 status;
> >
> > This looks bogus. An array is inline, a pointer points elsewhere -
> > they're not interchangeable.
>
> Yep; but in this case these are the only places in the code where _u32arr_ is
> being used:
>
> 3707         /* first pull in the header so we know the buffer length */
> 3708         for (bi = 0; bi < dword_len; bi++) {
> 3709                 bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> 3710                 le32_to_cpus(&bp->u32arr[bi]);
> 3711         }

So now line 3709 means: Read a pointer from bp->u32arr (the value
being read from there is not actually a valid pointer), and write to
that pointer at offset `bi`. I don't see how that line could execute
without crashing.