netdev - RE: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

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: <91c86d46b724411d9f788396816be30d@EX13D11EUB002.ant.amazon.com>
Date:   Thu, 20 Aug 2020 12:13:15 +0000
From:   "Jubran, Samih" <sameehj@...zon.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Machulsky, Zorik" <zorik@...zon.com>,
        "Matushevsky, Alexander" <matua@...zon.com>,
        "Bshara, Saeed" <saeedb@...zon.com>,
        "Wilson, Matt" <msw@...zon.com>,
        "Liguori, Anthony" <aliguori@...zon.com>,
        "Bshara, Nafea" <nafea@...zon.com>,
        "Tzalik, Guy" <gtzalik@...zon.com>,
        "Belgazal, Netanel" <netanel@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Herrenschmidt, Benjamin" <benh@...zon.com>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>,
        "Dagan, Noam" <ndagan@...zon.com>
Subject: RE: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer
 arithmetics



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Wednesday, August 19, 2020 5:17 PM
> To: Jubran, Samih <sameehj@...zon.com>
> Cc: davem@...emloft.net; netdev@...r.kernel.org; Woodhouse, David
> <dwmw@...zon.co.uk>; Machulsky, Zorik <zorik@...zon.com>;
> Matushevsky, Alexander <matua@...zon.com>; Bshara, Saeed
> <saeedb@...zon.com>; Wilson, Matt <msw@...zon.com>; Liguori,
> Anthony <aliguori@...zon.com>; Bshara, Nafea <nafea@...zon.com>;
> Tzalik, Guy <gtzalik@...zon.com>; Belgazal, Netanel
> <netanel@...zon.com>; Saidi, Ali <alisaidi@...zon.com>; Herrenschmidt,
> Benjamin <benh@...zon.com>; Kiyanovski, Arthur
> <akiyano@...zon.com>; Dagan, Noam <ndagan@...zon.com>
> Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use
> unsigned long for pointer arithmetics
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@...zon.com wrote:
> > From: Sameeh Jubran <sameehj@...zon.com>
> >
> > unsigned long is the type for doing maths on pointers.
> 
> Maths on pointers is perfectly valid. The real issue here is you have all your
> types mixed up.

The stat_offset field has the bytes from the start of the struct, the math is perfectly valid IMO¸
I have also went for the extra step and tested it using prints.

> 
> > -                     ptr = (u64 *)((uintptr_t)&ring->tx_stats +
> > -                             (uintptr_t)ena_stats->stat_offset);
> > +                     ptr = (u64 *)((unsigned long)&ring->tx_stats +
> > +                             ena_stats->stat_offset);
> 
> struct ena_ring {
> ...
>         union {
>                 struct ena_stats_tx tx_stats;
>                 struct ena_stats_rx rx_stats;
>         };
> 
> struct ena_stats_tx {
>         u64 cnt;
>         u64 bytes;
>         u64 queue_stop;
>         u64 prepare_ctx_err;
>         u64 queue_wakeup;
>         ...
> }
> 
> &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that,
> adding 1 for example, takes you forward a full ena_stats_tx structure. Not
> what you want.
> 
> &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give
> you bytes, etc.


If I understand you well, the alternative approach you are suggesting is:

ptr = &ring->tx_stats.cnt + ena_stats->stat_offset;

of course we need to convert the stat_offset field to be in 8 bytes resolution instead.

This approach has a potential bug hidden in it. If in the future
someone decides to expand the "ena_stats_tx" struct and add a field preceding cnt,
cnt will no longer be the beginning of the struct, which will cause a bug."

Therefore, if you have another way to do this, please share it. Otherwise I'd
rather leave this code as it is for the sake of robustness.

> 
>      Andrew