Nikolay Aleksandrov
2017-Jun-05 22:05 UTC
[Bridge] [PATCH] net: bridge: fix potential NULL pointer dereference
On 06/06/17 00:30, Gustavo A. R. Silva wrote:> Add NULL check before dereferencing pointer _p_ inside br_afspec(). > > Addresses-Coverity-ID: 1401872 > Signed-off-by: Gustavo A. R. Silva <garsilva at embeddedor.com> > --- > net/bridge/br_netlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 1e63ec4..ad85a9c 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -776,7 +776,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags) > goto out; > } > > - if (afspec) { > + if (p && afspec) { > err = br_afspec((struct net_bridge *)netdev_priv(dev), p, > afspec, RTM_SETLINK); > } >While I see a possible issue with the new bridge tunnel code (+CC Roopa), this is the wrong fix because there are legitimate use cases where p is null and br_afspec is called. We need to change the p->flags check in br_afspec()'s IFLA_BRIDGE_VLAN_TUNNEL_INFO case to check for a NULL p first. Thanks for the report!
Nikolay Aleksandrov
2017-Jun-05 22:26 UTC
[Bridge] [PATCH net] net: bridge: fix a null pointer dereference in br_afspec
We might call br_afspec() with p == NULL which is a valid use case if
the action is on the bridge device itself, but the bridge tunnel code
dereferences the p pointer without checking, so check if p is null
first.
Reported-by: Gustavo A. R. Silva <garsilva at embeddedor.com>
Fixes: efa5356b0d97 ("bridge: per vlan dst_metadata netlink support")
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
net/bridge/br_netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 1e63ec466d7c..3bcda556971e 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -595,7 +595,7 @@ static int br_afspec(struct net_bridge *br,
err = 0;
switch (nla_type(attr)) {
case IFLA_BRIDGE_VLAN_TUNNEL_INFO:
- if (!(p->flags & BR_VLAN_TUNNEL))
+ if (!p || !(p->flags & BR_VLAN_TUNNEL))
return -EINVAL;
err = br_parse_vlan_tunnel_info(attr, &tinfo_curr);
if (err)
--
2.1.4
Gustavo A. R. Silva
2017-Jun-06 03:13 UTC
[Bridge] [PATCH] net: bridge: fix potential NULL pointer dereference
Hi Nikolay, Quoting Nikolay Aleksandrov <nikolay at cumulusnetworks.com>:> On 06/06/17 00:30, Gustavo A. R. Silva wrote: >> Add NULL check before dereferencing pointer _p_ inside br_afspec(). >> >> Addresses-Coverity-ID: 1401872 >> Signed-off-by: Gustavo A. R. Silva <garsilva at embeddedor.com> >> --- >> net/bridge/br_netlink.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >> index 1e63ec4..ad85a9c 100644 >> --- a/net/bridge/br_netlink.c >> +++ b/net/bridge/br_netlink.c >> @@ -776,7 +776,7 @@ int br_setlink(struct net_device *dev, struct >> nlmsghdr *nlh, u16 flags) >> goto out; >> } >> >> - if (afspec) { >> + if (p && afspec) { >> err = br_afspec((struct net_bridge *)netdev_priv(dev), p, >> afspec, RTM_SETLINK); >> } >> > > While I see a possible issue with the new bridge tunnel code (+CC > Roopa), this is > the wrong fix because there are legitimate use cases where p is null > and br_afspec > is called. > We need to change the p->flags check in br_afspec()'s > IFLA_BRIDGE_VLAN_TUNNEL_INFO case > to check for a NULL p first. >You're right. I got it.> Thanks for the report!Sure thing, glad to help. Thank you! -- Gustavo A. R. Silva