netdev - Re: [PATCH net-next v14 12/13] net: ethtool: strset: Allow querying phy stats by index

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: <20240703085515.25dab47c@fedora-2.home>
Date: Wed, 3 Jul 2024 08:55:15 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Simon Horman <horms@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Russell King
 <linux@...linux.org.uk>, linux-arm-kernel@...ts.infradead.org, Christophe
 Leroy <christophe.leroy@...roup.eu>, Herve Codina
 <herve.codina@...tlin.com>, Florian Fainelli <f.fainelli@...il.com>, Heiner
 Kallweit <hkallweit1@...il.com>, Vladimir Oltean <vladimir.oltean@....com>,
 Köry Maincent <kory.maincent@...tlin.com>, Jesse Brandeburg
 <jesse.brandeburg@...el.com>, Marek Behún
 <kabel@...nel.org>, Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>, Nicolò Veronese
 <nicveronese@...il.com>, mwojtas@...omium.org, Nathan Chancellor
 <nathan@...nel.org>, Antoine Tenart <atenart@...nel.org>, Marc Kleine-Budde
 <mkl@...gutronix.de>
Subject: Re: [PATCH net-next v14 12/13] net: ethtool: strset: Allow querying
 phy stats by index

Hello Simon,

On Tue, 2 Jul 2024 11:54:11 +0100
Simon Horman <horms@...nel.org> wrote:

> On Mon, Jul 01, 2024 at 03:17:58PM +0200, Maxime Chevallier wrote:
> > The ETH_SS_PHY_STATS command gets PHY statistics. Use the phydev pointer
> > from the ethnl request to allow query phy stats from each PHY on the
> > link.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> > ---
> >  net/ethtool/strset.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c  
> 
> ...
> 
> > @@ -279,6 +280,8 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
> >  	const struct strset_req_info *req_info = STRSET_REQINFO(req_base);
> >  	struct strset_reply_data *data = STRSET_REPDATA(reply_base);
> >  	struct net_device *dev = reply_base->dev;
> > +	struct nlattr **tb = info->attrs;  
> 
> Hi Maxime,
> 
> Elsewhere in this function it is assumed that info may be NULL.
> But here it is dereferenced unconditionally.

Hmm in almst all netlink commands we do dereference the genl_info *info
pointer without checks.

I've looked into net/netlink/genetlink.c to backtrack call-sites and it
looks to be that indeed info can't be NULL (either populated from
genl_start() or genl_family_rcv_msg_doit(). Maybe Jakub can confirm
this ?

If what I say above is correct, I can include a small patch to remove
the un-necessary check that makes smatch think the genl_info pointer can
be NULL.

Thanks for the report,

Maxime