[PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Vladimir Oltean]

When a driver returns -EOPNOTSUPP in dsa_port_bridge_join() but failed
to provide a reason for it, DSA attempts to set the extack to say that
software fallback will kick in.

The problem is, when we use brctl and the legacy bridge ioctls, the
extack will be NULL, and DSA dereferences it in the process of setting
it.

Sergei Antonov proves this using the following stack trace:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
PC is at dsa_slave_changeupper+0x5c/0x158

 dsa_slave_changeupper from raw_notifier_call_chain+0x38/0x6c
 raw_notifier_call_chain from __netdev_upper_dev_link+0x198/0x3b4
 __netdev_upper_dev_link from netdev_master_upper_dev_link+0x50/0x78
 netdev_master_upper_dev_link from br_add_if+0x430/0x7f4
 br_add_if from br_ioctl_stub+0x170/0x530
 br_ioctl_stub from br_ioctl_call+0x54/0x7c
 br_ioctl_call from dev_ifsioc+0x4e0/0x6bc
 dev_ifsioc from dev_ioctl+0x2f8/0x758
 dev_ioctl from sock_ioctl+0x5f0/0x674
 sock_ioctl from sys_ioctl+0x518/0xe40
 sys_ioctl from ret_fast_syscall+0x0/0x1c

Fix the problem by only overriding the extack if non-NULL.

Fixes: 1c6e8088d9a7 ("net: dsa: allow port_bridge_join() to override extack message")
Link: https://lore.kernel.org/netdev/CABikg9wx7vB5eRDAYtvAm7fprJ09Ta27a4ZazC=NX5K4wn6pWA@mail.gmail.com/
Reported-by: Sergei Antonov <saproj@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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");
 				err = 0;
-- 
2.34.1

Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Florian Fainelli]

On 8/19/2022 10:39 AM, Vladimir Oltean wrote:
> When a driver returns -EOPNOTSUPP in dsa_port_bridge_join() but failed
> to provide a reason for it, DSA attempts to set the extack to say that
> software fallback will kick in.
> 
> The problem is, when we use brctl and the legacy bridge ioctls, the
> extack will be NULL, and DSA dereferences it in the process of setting
> it.
> 
> Sergei Antonov proves this using the following stack trace:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> PC is at dsa_slave_changeupper+0x5c/0x158
> 
>   dsa_slave_changeupper from raw_notifier_call_chain+0x38/0x6c
>   raw_notifier_call_chain from __netdev_upper_dev_link+0x198/0x3b4
>   __netdev_upper_dev_link from netdev_master_upper_dev_link+0x50/0x78
>   netdev_master_upper_dev_link from br_add_if+0x430/0x7f4
>   br_add_if from br_ioctl_stub+0x170/0x530
>   br_ioctl_stub from br_ioctl_call+0x54/0x7c
>   br_ioctl_call from dev_ifsioc+0x4e0/0x6bc
>   dev_ifsioc from dev_ioctl+0x2f8/0x758
>   dev_ioctl from sock_ioctl+0x5f0/0x674
>   sock_ioctl from sys_ioctl+0x518/0xe40
>   sys_ioctl from ret_fast_syscall+0x0/0x1c
> 
> Fix the problem by only overriding the extack if non-NULL.
> 
> Fixes: 1c6e8088d9a7 ("net: dsa: allow port_bridge_join() to override extack message")
> Link: https://lore.kernel.org/netdev/CABikg9wx7vB5eRDAYtvAm7fprJ09Ta27a4ZazC=NX5K4wn6pWA@mail.gmail.com/
> Reported-by: Sergei Antonov <saproj@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Sergei Antonov]

On Fri, 19 Aug 2022 at 20:39, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> When a driver returns -EOPNOTSUPP in dsa_port_bridge_join() but failed
> to provide a reason for it, DSA attempts to set the extack to say that
> software fallback will kick in.
>
> The problem is, when we use brctl and the legacy bridge ioctls, the
> extack will be NULL, and DSA dereferences it in the process of setting
> it.
>
> Sergei Antonov proves this using the following stack trace:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> PC is at dsa_slave_changeupper+0x5c/0x158
>
>  dsa_slave_changeupper from raw_notifier_call_chain+0x38/0x6c
>  raw_notifier_call_chain from __netdev_upper_dev_link+0x198/0x3b4
>  __netdev_upper_dev_link from netdev_master_upper_dev_link+0x50/0x78
>  netdev_master_upper_dev_link from br_add_if+0x430/0x7f4
>  br_add_if from br_ioctl_stub+0x170/0x530
>  br_ioctl_stub from br_ioctl_call+0x54/0x7c
>  br_ioctl_call from dev_ifsioc+0x4e0/0x6bc
>  dev_ifsioc from dev_ioctl+0x2f8/0x758
>  dev_ioctl from sock_ioctl+0x5f0/0x674
>  sock_ioctl from sys_ioctl+0x518/0xe40
>  sys_ioctl from ret_fast_syscall+0x0/0x1c
>
> Fix the problem by only overriding the extack if non-NULL.
>
> Fixes: 1c6e8088d9a7 ("net: dsa: allow port_bridge_join() to override extack message")
> Link: https://lore.kernel.org/netdev/CABikg9wx7vB5eRDAYtvAm7fprJ09Ta27a4ZazC=NX5K4wn6pWA@mail.gmail.com/
> Reported-by: Sergei Antonov <saproj@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Tested-by: Sergei Antonov <saproj@gmail.com>

Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Jakub Kicinski]

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,
which has the same effect. 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.

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.

Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Vladimir Oltean]

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.

Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Jakub Kicinski]

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.

Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 19 Aug 2022 20:39:25 +0300 you wrote:
> When a driver returns -EOPNOTSUPP in dsa_port_bridge_join() but failed
> to provide a reason for it, DSA attempts to set the extack to say that
> software fallback will kick in.
> 
> The problem is, when we use brctl and the legacy bridge ioctls, the
> extack will be NULL, and DSA dereferences it in the process of setting
> it.
> 
> [...]

Here is the summary with links:
  - [net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()
    https://git.kernel.org/netdev/net/c/855a28f9c96c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html