netdev - Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV

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: <20240827211224.0d172e40@samweis>
Date: Tue, 27 Aug 2024 21:12:24 +0200
From: Thomas Bogendoerfer <tbogendoerfer@...e.de>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, Dave Ertman <david.m.ertman@...el.com>,
 "Jiri Pirko" <jiri@...nulli.us>
Subject: Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support
 SRIOV_LAG

On Tue, 27 Aug 2024 09:16:51 +0200
Przemek Kitszel <przemyslaw.kitszel@...el.com> wrote:

> On 8/26/24 12:17, Thomas Bogendoerfer wrote:
> > On Mon, 26 Aug 2024 11:41:19 +0200
> > Jiri Pirko <jiri@...nulli.us> wrote:
> >   
> >> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@...e.de wrote:  
> >>> For PFs, which don't support SRIOV_LAG, there is no pf->lag struct
> >>> allocated. So before accessing pf->lag a NULL pointer check is needed.
> >>>
> >>> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@...e.de>  
> >>
> >> You need to add a "fixes" tag blaming the commit that introduced the
> >> bug.  
> 
> Would be also good to CC the author.

sure, I'm using get_maintainer for building address line and looks
like it only adds the author, if there is a Fixes tag, which IMHO
makes more sense than mailing all possible authors of file (in this
case it would work, but there are other files).

> > Fixes: 1e0f9881ef79 ("ice: Flesh out implementation of support for
> > SRIOV on bonded interface")  
> 
> the bug was introduced later, the tag should be:
> Fixes: ec5a6c5f79ed ("ice: process events created by lag netdev event 
> handler")

I'd like to disagree, ec5a6c5f79ed adds an empty ice_lag_move_new_vf_nodes(),
which will do no harm if pf->lag is NULL. Commit 1e0f9881ef79 introduces
the access to pf->lag without checking for NULL.
> 
> The mentioned commit extracted code into ice_lag_move_new_vf_nodes(),
> and there is just one call to this function by now, just after
> releasing lag_mutex, so would be good to change the semantics of
> ice_lag_move_new_vf_nodes() to "only for lag-enabled flows, with
> lag_mutex held", and fix the call to it to reflect that.

I could do that for sure, but IMHO this is about fixing a bug,
which crashes the kernel. Making the code better should be done
after fixing.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich