bridge: Fix missing Netlink message validations

PATCH In review Thomas Graf 4 participants 15 messages Nov 26, 2014 – Nov 29, 2014

Subsystems (5)

Networking Network drivers Intel Ethernet Ethernet bridge +1 more subsystems

lore

Share & tools

lore

Thomas Graf

author

2014-11-26 12:42 UTC #0 lore raw

cover bridge: Fix missing Netlink message validations

Adds various missing length checks in the bridging code for Netlink messages and corresponding attributes provided by user space.

Thomas Graf (5):
bridge: Validate IFLA_BRIDGE_FLAGS attribute length
net: Validate IFLA_BRIDGE_MODE attribute length
net: Check for presence of IFLA_AF_SPEC
bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK

 drivers/net/ethernet/emulex/benet/be_main.c   |  5 +++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  5 +++++
 net/bridge/br_netlink.c                       |  1 +
 net/core/rtnetlink.c                          | 23 ++++++++++++++++++-----
 4 files changed, 29 insertions(+), 5 deletions(-)

Signature · 1 line

1.9.3

Thomas Graf

author

2014-11-26 12:42 UTC #1 lore raw

PATCH 1/5 bridge: Validate IFLA_BRIDGE_FLAGS attribute length

Payload is currently accessed blindly and may exceed valid message boundaries.

Fixes 407af3299 ("bridge: Add netlink interface to configure vlans on bridge ports")

Cc Vlad Yasevich <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

---
 net/core/rtnetlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

net/core/rtnetlink.c +6 -0

index a688268..5a853f8 100644
@@ -2798,6 +2798,9 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (br_spec) {
 		nla_for_each_nested(attr, br_spec, rem) {
 			if (nla_type(attr) == IFLA_BRIDGE_FLAGS) {
+				if (nla_len(attr) < sizeof(flags))
+					return -EINVAL;
+
 				have_flags = true;
 				flags = nla_get_u16(attr);
 				break;

@@ -2868,6 +2871,9 @@ static int rtnl_bridge_dellink(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (br_spec) {
 		nla_for_each_nested(attr, br_spec, rem) {
 			if (nla_type(attr) == IFLA_BRIDGE_FLAGS) {
+				if (nla_len(attr) < sizeof(flags))
+					return -EINVAL;
+
 				have_flags = true;
 				flags = nla_get_u16(attr);
 				break;

Signature · 1 line

1.9.3

Thomas Graf

author

2014-11-26 12:42 UTC #2 lore raw

PATCH 2/5 net: Validate IFLA_BRIDGE_MODE attribute length

In reply to Thomas Graf

Payload is currently accessed blindly and may exceed valid message boundaries.

Fixes a77dcb8c8 ("be2net: set and query VEB/VEPA mode of the PF interface")

Fixes 815cccbf1 ("ixgbe: add setlink, getlink support to ixgbe and ixgbevf")

Cc Ajit Khaparde <[email protected]>

Cc John Fastabend <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

---
 drivers/net/ethernet/emulex/benet/be_main.c   | 3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
 2 files changed, 6 insertions(+)

drivers/net/ethernet/emulex/benet/be_main.c +3 -0

index 3e8475c..337e4cd 100644
@@ -4314,6 +4314,9 @@ static int be_ndo_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh)
 		if (nla_type(attr) != IFLA_BRIDGE_MODE)
 			continue;
 
+		if (nla_len(attr) < sizeof(mode))
+			return -EINVAL;
+
 		mode = nla_get_u16(attr);
 		if (mode != BRIDGE_MODE_VEPA && mode != BRIDGE_MODE_VEB)
 			return -EINVAL;

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +3 -0

index 82ffe8b..dff9905 100644
@@ -7677,6 +7677,9 @@ static int ixgbe_ndo_bridge_setlink(struct net_device *dev,
 		if (nla_type(attr) != IFLA_BRIDGE_MODE)
 			continue;
 
+		if (nla_len(attr) < sizeof(mode))
+			return -EINVAL;
+
 		mode = nla_get_u16(attr);
 		if (mode == BRIDGE_MODE_VEPA) {
 			reg = 0;

Signature · 1 line

1.9.3

Jeff Kirsher

Acked-by on v1 2/5

2014-11-26 13:29 UTC #3 lore raw

Re: [PATCH 2/5] net: Validate IFLA_BRIDGE_MODE attribute length

Quoted 8 lines · Thomas Graf

Payload is currently accessed blindly and may exceed valid message boundaries.
Fixes a77dcb8c8 ("be2net: set and query VEB/VEPA mode of the PF interface")
Fixes 815cccbf1 ("ixgbe: add setlink, getlink support to ixgbe and ixgbevf")
Cc Ajit Khaparde <[email protected]>
Cc John Fastabend <[email protected]>
Signed-off-by Thomas Graf <[email protected]>

Quoted 5 lines

---
 drivers/net/ethernet/emulex/benet/be_main.c   | 3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
 2 files changed, 6 insertions(+)

Acked-by Jeff Kirsher <[email protected]>

John Fastabend

Acked-by on v1 2/5

2014-11-26 16:43 UTC #4 lore raw

Re: [PATCH 2/5] net: Validate IFLA_BRIDGE_MODE attribute length

In reply to Thomas Graf

Quoted 8 lines · Thomas Graf

Payload is currently accessed blindly and may exceed valid message boundaries.
Fixes a77dcb8c8 ("be2net: set and query VEB/VEPA mode of the PF interface")
Fixes 815cccbf1 ("ixgbe: add setlink, getlink support to ixgbe and ixgbevf")
Cc Ajit Khaparde <[email protected]>
Cc John Fastabend <[email protected]>
Signed-off-by Thomas Graf <[email protected]>

Quoted 5 lines

---
  drivers/net/ethernet/emulex/benet/be_main.c   | 3 +++
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
  2 files changed, 6 insertions(+)

Thanks Thomas.

Acked-by John Fastabend <[email protected]>

Signature · 1 line

John Fastabend         Intel Corporation

Thomas Graf

author

2014-11-26 12:42 UTC #5 lore raw

PATCH 3/5 net: Check for presence of IFLA_AF_SPEC

In reply to Thomas Graf

ndo_bridge_setlink() is currently only called on the slave if IFLA_AF_SPEC is set but this is a very fragile assumption and may change in the future.

Cc Ajit Khaparde <[email protected]>

Cc John Fastabend <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

---
 drivers/net/ethernet/emulex/benet/be_main.c   | 2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
 2 files changed, 4 insertions(+)

drivers/net/ethernet/emulex/benet/be_main.c +2 -0

index 337e4cd..597c463 100644
@@ -4309,6 +4309,8 @@ static int be_ndo_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh)
 		return -EOPNOTSUPP;
 
 	br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
+	if (!br_spec)
+		return -EINVAL;
 
 	nla_for_each_nested(attr, br_spec, rem) {
 		if (nla_type(attr) != IFLA_BRIDGE_MODE)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +2 -0

index dff9905..cc51554 100644
@@ -7669,6 +7669,8 @@ static int ixgbe_ndo_bridge_setlink(struct net_device *dev,
 		return -EOPNOTSUPP;
 
 	br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
+	if (!br_spec)
+		return -EINVAL;
 
 	nla_for_each_nested(attr, br_spec, rem) {
 		__u16 mode;

Signature · 1 line

1.9.3

Jeff Kirsher

Acked-by on v1 3/5

2014-11-26 13:30 UTC #6 lore raw

Re: [PATCH 3/5] net: Check for presence of IFLA_AF_SPEC

Quoted 4 lines · Thomas Graf

ndo_bridge_setlink() is currently only called on the slave if IFLA_AF_SPEC is set but this is a very fragile assumption and may change in the future.

Quoted 8 lines

Cc Ajit Khaparde <[email protected]>
Cc John Fastabend <[email protected]>
Signed-off-by Thomas Graf <[email protected]>
---
 drivers/net/ethernet/emulex/benet/be_main.c   | 2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
 2 files changed, 4 insertions(+)

Acked-by Jeff Kirsher <[email protected]>

Thomas Graf

author

2014-11-26 12:42 UTC #7 lore raw

PATCH 4/5 bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE

In reply to Thomas Graf

Fixes c2d3babf ("bridge: implement multicast fast leave")

Cc David S. Miller <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

---
 net/bridge/br_netlink.c | 1 +
 1 file changed, 1 insertion(+)

net/bridge/br_netlink.c +1 -0

index 2ff9706..e5ec470 100644
@@ -280,6 +280,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
 	[IFLA_BRPORT_MODE]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_GUARD]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_PROTECT]	= { .type = NLA_U8 },
+	[IFLA_BRPORT_FAST_LEAVE]= { .type = NLA_U8 },
 	[IFLA_BRPORT_LEARNING]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_UNICAST_FLOOD] = { .type = NLA_U8 },
 };

Signature · 1 line

1.9.3

Thomas Graf

author

2014-11-26 12:42 UTC #8 lore raw

PATCH 5/5 bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK

In reply to Thomas Graf

Only search for IFLA_EXT_MASK if the message actually carries a ifinfomsg header and validate minimal length requirements for IFLA_EXT_MASK.

Fixes 6cbdceeb ("bridge: Dump vlan information from a bridge port")

Cc Vlad Yasevich <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

---
 net/core/rtnetlink.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

net/core/rtnetlink.c +12 -5

index 5a853f8..b9b7dfa 100644
@@ -2685,13 +2685,20 @@ static int rtnl_bridge_getlink(struct sk_buff *skb, struct netlink_callback *cb)
 	int idx = 0;
 	u32 portid = NETLINK_CB(cb->skb).portid;
 	u32 seq = cb->nlh->nlmsg_seq;
-	struct nlattr *extfilt;
 	u32 filter_mask = 0;
 
-	extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg),
-				  IFLA_EXT_MASK);
-	if (extfilt)
-		filter_mask = nla_get_u32(extfilt);
+	if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
+		struct nlattr *extfilt;
+
+		extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg),
+					  IFLA_EXT_MASK);
+		if (extfilt) {
+			if (nla_len(extfilt) < sizeof(filter_mask))
+				return -EINVAL;
+
+			filter_mask = nla_get_u32(extfilt);
+		}
+	}
 
 	rcu_read_lock();
 	for_each_netdev_rcu(net, dev) {

Signature · 1 line

1.9.3

John Fastabend

2014-11-26 16:58 UTC #9 lore raw

In reply to Thomas Graf

Quoted 10 lines · Thomas Graf

Adds various missing length checks in the bridging code for Netlink messages and corresponding attributes provided by user space.
Thomas Graf (5):
bridge: Validate IFLA_BRIDGE_FLAGS attribute length
net: Validate IFLA_BRIDGE_MODE attribute length
net: Check for presence of IFLA_AF_SPEC
bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK

Quoted 6 lines

  drivers/net/ethernet/emulex/benet/be_main.c   |  5 +++++
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  5 +++++
  net/bridge/br_netlink.c                       |  1 +
  net/core/rtnetlink.c                          | 23 ++++++++++++++++++-----
  4 files changed, 29 insertions(+), 5 deletions(-)

+Jiri

Looks like a miss in bond_netlink also? Seems like writing a smatch or cocci check for this would be worthwhile.

Quoted 19 lines

drivers/net/bonding/bond_netlink.c +6 -1

index 3e6eebd..7b11243 100644
@@ -225,7 +225,12 @@ static int bond_changelink(struct net_device *bond_dev,
 
                bond_option_arp_ip_targets_clear(bond);
                nla_for_each_nested(attr, data[IFLA_BOND_ARP_IP_TARGET], rem) {
-                       __be32 target = nla_get_be32(attr);
+                       __be32 target;
+
+                       if (nla_len(attr) < sizeof(target))
+                               return -EINVAL;
+
+                       target = nla_get_be32(attr);
 
                        bond_opt_initval(&newval, (__force u64)target);
                        err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,

Signature · 1 line

John Fastabend         Intel Corporation

Thomas Graf

author

2014-11-26 17:06 UTC #10 lore raw

Quoted 18 lines · John Fastabend

Quoted 10 lines · Thomas Graf
Adds various missing length checks in the bridging code for Netlink messages and corresponding attributes provided by user space.
Thomas Graf (5):
bridge: Validate IFLA_BRIDGE_FLAGS attribute length
net: Validate IFLA_BRIDGE_MODE attribute length
net: Check for presence of IFLA_AF_SPEC
bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
Quoted 6 lines
 drivers/net/ethernet/emulex/benet/be_main.c   |  5 +++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  5 +++++
 net/bridge/br_netlink.c                       |  1 +
 net/core/rtnetlink.c                          | 23 ++++++++++++++++++-----
 4 files changed, 29 insertions(+), 5 deletions(-)

Quoted 4 lines

+Jiri
Looks like a miss in bond_netlink also? Seems like writing a smatch or cocci check for this would be worthwhile.

Thanks, I'll take a look.

The cocci check is somewhat difficult as validation is often centralized and decoupled from actual access to implement atomic operations. I'll give it a try though.

John Fastabend

2014-11-26 17:26 UTC #11 lore raw

Quoted 21 lines · Thomas Graf

Quoted 20 lines · John Fastabend
Quoted 10 lines · Thomas Graf
Adds various missing length checks in the bridging code for Netlink messages and corresponding attributes provided by user space.
Thomas Graf (5):
bridge: Validate IFLA_BRIDGE_FLAGS attribute length
net: Validate IFLA_BRIDGE_MODE attribute length
net: Check for presence of IFLA_AF_SPEC
bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
Quoted 6 lines
  drivers/net/ethernet/emulex/benet/be_main.c   |  5 +++++
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  5 +++++
  net/bridge/br_netlink.c                       |  1 +
  net/core/rtnetlink.c                          | 23 ++++++++++++++++++-----
  4 files changed, 29 insertions(+), 5 deletions(-)
+Jiri

Quoted 9 lines

Quoted 2 lines
Looks like a miss in bond_netlink also? Seems like writing a smatch or cocci check for this would be worthwhile.
Thanks, I'll take a look.
The cocci check is somewhat difficult as validation is often centralized and decoupled from actual access to implement atomic operations. I'll give it a try though.

Sounds good, if I get some time I could give it a try as well.

Also another missing one here or at least I'm not seeing it if its there,

Quoted 13 lines

net/ipv4/devinet.c +4 -1

@@ -1687,8 +1687,11 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
                BUG();
 
        if (tb[IFLA_INET_CONF]) {
-               nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
+               nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+                       if (nla_len(a) < sizeof(u32))
+                               return -EINVAL;
                        ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+               }
        }

Signature · 1 line

John Fastabend         Intel Corporation

Thomas Graf

author

2014-11-26 23:14 UTC #12 lore raw

Quoted 5 lines · John Fastabend

Quoted 5 lines

net/ipv4/devinet.c +0 -0

@@ -1687,8 +1687,11 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
               BUG();

Quoted 7 lines

net/ipv4/devinet.c static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla) · line 1694

Quoted 7 lines

net/ipv4/devinet.c +4 -1

       if (tb[IFLA_INET_CONF]) {
-               nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
+               nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+                       if (nla_len(a) < sizeof(u32))
+                               return -EINVAL;
                       ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+               }

Looked into this and found a validation function inet_validate_link_af(). It's split to keep the updates atomic.

John Fastabend

2014-11-29 17:51 UTC #13 lore raw

Quoted 6 lines · Thomas Graf

Quoted 5 lines · John Fastabend

Quoted 5 lines

net/ipv4/devinet.c +0 -0

@@ -1687,8 +1687,11 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
                BUG();

Quoted 11 lines

Quoted 7 lines

net/ipv4/devinet.c static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla) · line 1694

Quoted 7 lines

net/ipv4/devinet.c +4 -1

        if (tb[IFLA_INET_CONF]) {
-               nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
+               nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+                       if (nla_len(a) < sizeof(u32))
+                               return -EINVAL;
                        ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+               }

Looked into this and found a validation function inet_validate_link_af(). It's split to keep the updates atomic.

Ah great thanks.

Signature · 1 line

John Fastabend         Intel Corporation

David Miller

maintainer

2014-11-26 20:29 UTC #14 lore raw

In reply to Thomas Graf

From: Thomas Graf <[email protected]>
Date: Wed, 26 Nov 2014 13:42:15 +0100

Quoted 2 lines

Adds various missing length checks in the bridging code for Netlink messages and corresponding attributes provided by user space.

Series applied, thanks Thomas.