[PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()

[PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()

[Garry McNulty]

br_port_get_rtnl() can return NULL if the network device is not a bridge
port (IFF_BRIDGE_PORT flag not set). br_port_slave_changelink() and
br_port_fill_slave_info() callbacks dereference this pointer without
checking. Currently this is not a problem because slave devices always
set this flag. Add null check in case these conditions ever change.

Detected by CoverityScan, CID 1339613 ("Dereference null return value")

Signed-off-by: Garry McNulty <garrmcnu@gmail.com>
---
 net/bridge/br_netlink.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 9f5eb05b0373..b3ad135b7157 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -947,13 +947,14 @@ static int br_port_slave_changelink(struct net_device *brdev,
 				    struct netlink_ext_ack *extack)
 {
 	struct net_bridge *br = netdev_priv(brdev);
+	struct net_bridge_port *p = br_port_get_rtnl(dev);
 	int ret;
 
-	if (!data)
+	if (!data || !p)
 		return 0;
 
 	spin_lock_bh(&br->lock);
-	ret = br_setport(br_port_get_rtnl(dev), data);
+	ret = br_setport(p, data);
 	spin_unlock_bh(&br->lock);
 
 	return ret;
@@ -963,7 +964,9 @@ static int br_port_fill_slave_info(struct sk_buff *skb,
 				   const struct net_device *brdev,
 				   const struct net_device *dev)
 {
-	return br_port_fill_attrs(skb, br_port_get_rtnl(dev));
+	struct net_bridge_port *p = br_port_get_rtnl(dev);
+
+	return p ? br_port_fill_attrs(skb, p) : -EINVAL;
 }
 
 static size_t br_port_get_slave_size(const struct net_device *brdev,
-- 
2.14.4

Re: [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()

[David Miller]

From: Garry McNulty <garrmcnu@gmail.com>
Date: Thu, 21 Jun 2018 21:14:27 +0100

> br_port_get_rtnl() can return NULL if the network device is not a bridge
> port (IFF_BRIDGE_PORT flag not set). br_port_slave_changelink() and
> br_port_fill_slave_info() callbacks dereference this pointer without
> checking. Currently this is not a problem because slave devices always
> set this flag. Add null check in case these conditions ever change.
> 
> Detected by CoverityScan, CID 1339613 ("Dereference null return value")
> 
> Signed-off-by: Garry McNulty <garrmcnu@gmail.com>

I don't think this is reasonable.

The bridge code will never, ever, install a slave that doesn't have
that bit set.  It's the most fundamental aspect of how these objects
are managed.

Re: [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()

[Stephen Hemminger]

On Fri, 22 Jun 2018 07:20:56 +0900 (KST)
David Miller <davem@davemloft.net> wrote:

> From: Garry McNulty <garrmcnu@gmail.com>
> Date: Thu, 21 Jun 2018 21:14:27 +0100
> 
> > br_port_get_rtnl() can return NULL if the network device is not a bridge
> > port (IFF_BRIDGE_PORT flag not set). br_port_slave_changelink() and
> > br_port_fill_slave_info() callbacks dereference this pointer without
> > checking. Currently this is not a problem because slave devices always
> > set this flag. Add null check in case these conditions ever change.
> > 
> > Detected by CoverityScan, CID 1339613 ("Dereference null return value")
> > 
> > Signed-off-by: Garry McNulty <garrmcnu@gmail.com>  
> 
> I don't think this is reasonable.
> 
> The bridge code will never, ever, install a slave that doesn't have
> that bit set.  It's the most fundamental aspect of how these objects
> are managed.

Agree with dave. Workarounds for static tools are ok if they don't introduce
other paths. But if your fix introduces another error path which can never
be reached, it is hurting not helping.

Re: [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()

[Nikolay Aleksandrov]

On 06/22/2018 01:20 AM, David Miller wrote:
> From: Garry McNulty <garrmcnu@gmail.com>
> Date: Thu, 21 Jun 2018 21:14:27 +0100
> 
>> br_port_get_rtnl() can return NULL if the network device is not a bridge
>> port (IFF_BRIDGE_PORT flag not set). br_port_slave_changelink() and
>> br_port_fill_slave_info() callbacks dereference this pointer without
>> checking. Currently this is not a problem because slave devices always
>> set this flag. Add null check in case these conditions ever changye.
>>
>> Detected by CoverityScan, CID 1339613 ("Dereference null return value")
>>
>> Signed-off-by: Garry McNulty <garrmcnu@gmail.com>
> 
> I don't think this is reasonable.
> 
> The bridge code will never, ever, install a slave that doesn't have
> that bit set.  It's the most fundamental aspect of how these objects
> are managed.
> 
+1

This keeps coming up, here's the previous one:
https://patchwork.ozlabs.org/patch/896046/

Please do a more thorough check if these conditions can actually occur.
In this case, as Dave said, they cannot.

To be explicit as with the patch I mentioned above:
Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

You can find more info in my reply to the patch above.

Thanks,
 Nik

Re: [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()

[Garry McNulty]

On Fri, 22 Jun 2018 at 00:35, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
>
> On 06/22/2018 01:20 AM, David Miller wrote:
> > From: Garry McNulty <garrmcnu@gmail.com>
> > Date: Thu, 21 Jun 2018 21:14:27 +0100
> >
> >> br_port_get_rtnl() can return NULL if the network device is not a bridge
> >> port (IFF_BRIDGE_PORT flag not set). br_port_slave_changelink() and
> >> br_port_fill_slave_info() callbacks dereference this pointer without
> >> checking. Currently this is not a problem because slave devices always
> >> set this flag. Add null check in case these conditions ever changye.
> >>
> >> Detected by CoverityScan, CID 1339613 ("Dereference null return value")
> >>
> >> Signed-off-by: Garry McNulty <garrmcnu@gmail.com>
> >
> > I don't think this is reasonable.
> >
> > The bridge code will never, ever, install a slave that doesn't have
> > that bit set.  It's the most fundamental aspect of how these objects
> > are managed.
> >
> +1
>
> This keeps coming up, here's the previous one:
> https://patchwork.ozlabs.org/patch/896046/
>
> Please do a more thorough check if these conditions can actually occur.
> In this case, as Dave said, they cannot.
>
> To be explicit as with the patch I mentioned above:
> Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
> You can find more info in my reply to the patch above.
>
> Thanks,
>  Nik

Thanks for reviewing and for the feedback.

Regards

Garry