netdev - Re: [net 01/12] net/mlx5e: Use flow keys dissector to parse packets for ARFS

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:   Fri, 9 Aug 2019 12:01:23 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Saeed Mahameed <saeedm@...lanox.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...lanox.com>,
        Maxim Mikityanskiy <maximmi@...lanox.com>
Subject: Re: [net 01/12] net/mlx5e: Use flow keys dissector to parse packets
 for ARFS

On Fri, 9 Aug 2019 18:49:50 +0000, Saeed Mahameed wrote:
> On Thu, 2019-08-08 at 18:15 -0700, Jakub Kicinski wrote:
> > On Thu, 8 Aug 2019 20:22:00 +0000, Saeed Mahameed wrote:  
> > > From: Maxim Mikityanskiy <maximmi@...lanox.com>
> > > 
> > > The current ARFS code relies on certain fields to be set in the SKB
> > > (e.g. transport_header) and extracts IP addresses and ports by
> > > custom
> > > code that parses the packet. The necessary SKB fields, however, are
> > > not
> > > always set at that point, which leads to an out-of-bounds access.
> > > Use
> > > skb_flow_dissect_flow_keys() to get the necessary information
> > > reliably,
> > > fix the out-of-bounds access and reuse the code.  
> > 
> > The whole series LGTM, FWIW.
> > 
> > I'd be curious to hear which path does not have the skb fully 
> > set up, could you elaborate? (I'm certainly no aRFC expert this
> > is pure curiosity).  
> 
> In our regression we found two use cases that might lead aRFS using un-
> initialized values.
> 1) GRO Disabled, Usually GRO fills the necessary fields.
> 2) Raw socket type of tests.
> 
> And i am sure there are many other use cases. So drivers must use
> skb_flow_dissect_flow_keys() for aRFS parsing and eliminate all
> uncertainties. 

Looking at the code now it makes perfect sense. We could probably
refactor to call the dissector in the core at some point.

Thanks for the explanation!