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]

Date:   Tue, 23 Aug 2022 08:00:23 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
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 Tue, 23 Aug 2022 10:08:34 +0000 Vladimir Oltean wrote:
> 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.

Hm, I was pretty sure that's what we did in tc, but maybe it was just
discussed and never done. Let me apply, then.

> > 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.