netdev - Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave

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: <20220823100834.qikdvkekg6swn7rb@skbuf>
Date:   Tue, 23 Aug 2022 10:08:34 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Clément Léger <clement.leger@...tlin.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Sergei Antonov <saproj@...il.com>
Subject: Re: [PATCH net] net: dsa: don't dereference NULL extack in
 dsa_slave_changeupper()

On Mon, Aug 22, 2022 at 06:25:23PM -0700, Jakub Kicinski wrote:
> On Fri, 19 Aug 2022 20:39:25 +0300 Vladimir Oltean wrote:
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > index c548b969b083..804a00324c8b 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -2487,7 +2487,7 @@ static int dsa_slave_changeupper(struct net_device *dev,
> >  			if (!err)
> >  				dsa_bridge_mtu_normalization(dp);
> >  			if (err == -EOPNOTSUPP) {
> > -				if (!extack->_msg)
> > +				if (extack && !extack->_msg)
> >  					NL_SET_ERR_MSG_MOD(extack,
> >  							   "Offloading not supported");
> 
> Other offload paths set the extack prior to the driver call,

Example?

> which has the same effect.

No, definitely not the same effect. The difference between (a) setting it
to "Offloading not supported" before the call to dsa_port_bridge_join()
and (b) setting it to "Offloading not supported" only if dsa_port_bridge_join()
returned -EOPNOTSUPP is that drivers don't have to set an extack message
if they return success, or if they don't implement ds->ops->port_bridge_join.
The behavior changes for a driver that doesn't set the extack but
returns 0 if I do that.

> Can't we do the same thing here?
> Do we care about preserving the extack from another notifier 
> handler or something? Does not seem like that's the case judging 
> but the commit under Fixes.

Preserving yes, from another notifier handler no.

DSA suppresses the -EOPNOTSUPP error code from this operation and
returns 0 to user space, along with a warning note via extack.

The driver's ds->ops->port_bridge_join() method is given an extack.
Therefore, if the driver has set the extack to anything, presumably it
is more specific than what DSA has to say.

> If it is the case (and hopefully not) we should add a new macro wrapper.
> Manually twiddling with a field starting with an underscore makes
> me feel dirty. Perhaps I have been writing too much python lately.

Ok, can do later (not "net" patch). Also, if you search for _msg in
net/dsa/ you'll find more occurrences of accessing it directly.