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]

Date:   Wed, 26 Aug 2020 10:48:47 +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: Jubran, Samih
> Sent: Thursday, August 20, 2020 3:13 PM
> To: 'Andrew Lunn' <andrew@...n.ch>
> 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
> 
> 
> 
> > -----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


Ping.