* [PATCH] net: bridge: add missing NULL checks @ 2018-04-08 17:49 Laszlo Toth 2018-04-08 22:25 ` Nikolay Aleksandrov 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Toth @ 2018-04-08 17:49 UTC (permalink / raw) To: Stephen Hemminger, David S. Miller; +Cc: bridge, netdev br_port_get_rtnl() can return NULL Signed-off-by: Laszlo Toth <laszlth@gmail.com> --- net/bridge/br_netlink.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 015f465c..cbec11f 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -939,14 +939,17 @@ static int br_port_slave_changelink(struct net_device *brdev, struct nlattr *data[], struct netlink_ext_ack *extack) { + struct net_bridge_port *port = br_port_get_rtnl(dev); struct net_bridge *br = netdev_priv(brdev); int ret; if (!data) return 0; + if (!port) + return -EINVAL; spin_lock_bh(&br->lock); - ret = br_setport(br_port_get_rtnl(dev), data); + ret = br_setport(port, data); spin_unlock_bh(&br->lock); return ret; @@ -956,7 +959,12 @@ 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 *port = br_port_get_rtnl(dev); + + if (!port) + return -EINVAL; + + return br_port_fill_attrs(skb, port); } static size_t br_port_get_slave_size(const struct net_device *brdev, -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net: bridge: add missing NULL checks 2018-04-08 17:49 [PATCH] net: bridge: add missing NULL checks Laszlo Toth @ 2018-04-08 22:25 ` Nikolay Aleksandrov 2018-04-10 17:22 ` Laszlo Toth 0 siblings, 1 reply; 3+ messages in thread From: Nikolay Aleksandrov @ 2018-04-08 22:25 UTC (permalink / raw) To: Laszlo Toth, Stephen Hemminger, David S. Miller; +Cc: netdev, bridge On 08/04/18 20:49, Laszlo Toth wrote: > br_port_get_rtnl() can return NULL > > Signed-off-by: Laszlo Toth <laszlth@gmail.com> > --- > net/bridge/br_netlink.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> More below. > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 015f465c..cbec11f 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -939,14 +939,17 @@ static int br_port_slave_changelink(struct net_device *brdev, > struct nlattr *data[], > struct netlink_ext_ack *extack) > { > + struct net_bridge_port *port = br_port_get_rtnl(dev); > struct net_bridge *br = netdev_priv(brdev); > int ret; > > if (!data) > return 0; > + if (!port) > + return -EINVAL; > If we're here, it means the master device of dev is a bridge => dev is a bridge port, since we're running with RTNL that cannot change, so this check is unnecessary. Have you actually hit a bug with this code ? > spin_lock_bh(&br->lock); > - ret = br_setport(br_port_get_rtnl(dev), data); > + ret = br_setport(port, data); > spin_unlock_bh(&br->lock); > > return ret; > @@ -956,7 +959,12 @@ 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 *port = br_port_get_rtnl(dev); > + > + if (!port) > + return -EINVAL; > + > + return br_port_fill_attrs(skb, port); Same rationale here, fill_slave_info is called via a master device's ops under RTNL, which means dev is a bridge port and that also cannot change. If you have hit a bug with this code, can we see the trace ? The problem might be elsewhere. Thanks, Nik > } > > static size_t br_port_get_slave_size(const struct net_device *brdev, > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net: bridge: add missing NULL checks 2018-04-08 22:25 ` Nikolay Aleksandrov @ 2018-04-10 17:22 ` Laszlo Toth 0 siblings, 0 replies; 3+ messages in thread From: Laszlo Toth @ 2018-04-10 17:22 UTC (permalink / raw) To: Nikolay Aleksandrov Cc: Laszlo Toth, Stephen Hemminger, David S. Miller, bridge, netdev On Mon, Apr 09, 2018 at 01:25:41AM +0300, Nikolay Aleksandrov wrote: > On 08/04/18 20:49, Laszlo Toth wrote: > >br_port_get_rtnl() can return NULL > > > >Signed-off-by: Laszlo Toth <laszlth@gmail.com> > >--- > > net/bridge/br_netlink.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > More below. > > >diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > >index 015f465c..cbec11f 100644 > >--- a/net/bridge/br_netlink.c > >+++ b/net/bridge/br_netlink.c > >@@ -939,14 +939,17 @@ static int br_port_slave_changelink(struct net_device *brdev, > > struct nlattr *data[], > > struct netlink_ext_ack *extack) > > { > >+ struct net_bridge_port *port = br_port_get_rtnl(dev); > > struct net_bridge *br = netdev_priv(brdev); > > int ret; > > if (!data) > > return 0; > >+ if (!port) > >+ return -EINVAL; > > If we're here, it means the master device of dev is a bridge => dev is a bridge port, > since we're running with RTNL that cannot change, so this check is unnecessary. > > Have you actually hit a bug with this code ? > > > spin_lock_bh(&br->lock); > >- ret = br_setport(br_port_get_rtnl(dev), data); > >+ ret = br_setport(port, data); > > spin_unlock_bh(&br->lock); > > return ret; > >@@ -956,7 +959,12 @@ 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 *port = br_port_get_rtnl(dev); > >+ > >+ if (!port) > >+ return -EINVAL; > >+ > >+ return br_port_fill_attrs(skb, port); > > Same rationale here, fill_slave_info is called via a master device's ops > under RTNL, which means dev is a bridge port and that also cannot change. > > If you have hit a bug with this code, can we see the trace ? > The problem might be elsewhere. There was a NULL dereference in br_port_fill_attrs(), but on a much older release w/ a probably buggy and custom driver, so there is no real problem to trace. Anyway I thought I'd make a quick patch from it, but you're right, it's pointless to validate twice. Please just ignore the patch. Laszlo > > Thanks, > Nik > > > } > > static size_t br_port_get_slave_size(const struct net_device *brdev, > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-04-10 17:22 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-04-08 17:49 [PATCH] net: bridge: add missing NULL checks Laszlo Toth 2018-04-08 22:25 ` Nikolay Aleksandrov 2018-04-10 17:22 ` Laszlo Toth
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).