David Miller
2018-Jun-21 22:20 UTC
[Bridge] [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()
From: Garry McNulty <garrmcnu at 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 at 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.
Stephen Hemminger
2018-Jun-21 23:21 UTC
[Bridge] [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()
On Fri, 22 Jun 2018 07:20:56 +0900 (KST) David Miller <davem at davemloft.net> wrote:> From: Garry McNulty <garrmcnu at 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 at 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.
Nikolay Aleksandrov
2018-Jun-21 23:35 UTC
[Bridge] [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()
On 06/22/2018 01:20 AM, David Miller wrote:> From: Garry McNulty <garrmcnu at 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 at 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 at cumulusnetworks.com> You can find more info in my reply to the patch above. Thanks, Nik