Re: [PATCH] net: bridge: add missing NULL checks
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> >--- > > net/bridge/br_netlink.c | 12 ++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > Nacked-by: Nikolay Aleksandrov > 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(>lock); > >-ret = br_setport(br_port_get_rtnl(dev), data); > >+ret = br_setport(port, data); > > spin_unlock_bh(>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, > > >
Re: [PATCH] net: bridge: add missing NULL checks
On 08/04/18 20:49, Laszlo Toth wrote: br_port_get_rtnl() can return NULL Signed-off-by: Laszlo Toth--- net/bridge/br_netlink.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) Nacked-by: Nikolay Aleksandrov 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(>lock); - ret = br_setport(br_port_get_rtnl(dev), data); + ret = br_setport(port, data); spin_unlock_bh(>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,
[PATCH] net: bridge: add missing NULL checks
br_port_get_rtnl() can return NULL Signed-off-by: Laszlo Toth--- 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(>lock); - ret = br_setport(br_port_get_rtnl(dev), data); + ret = br_setport(port, data); spin_unlock_bh(>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