[PATCH 0/5 net] bridge: Fix missing Netlink message validations

[PATCH 0/5 net] bridge: Fix missing Netlink message validations

[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

 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(-)

-- 
1.9.3

[PATCH 1/5] bridge: Validate IFLA_BRIDGE_FLAGS attribute length

[Thomas Graf]

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 <vyasevic@redhat.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/core/rtnetlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a688268..5a853f8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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;
-- 
1.9.3

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

[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 <ajit.khaparde@emulex.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 drivers/net/ethernet/emulex/benet/be_main.c   | 3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 3e8475c..337e4cd 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -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;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 82ffe8b..dff9905 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -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;
-- 
1.9.3

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

[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 <ajit.khaparde@emulex.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 drivers/net/ethernet/emulex/benet/be_main.c   | 2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 337e4cd..597c463 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -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)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index dff9905..cc51554 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -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;
-- 
1.9.3

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

[Thomas Graf]

Fixes: c2d3babf ("bridge: implement multicast fast leave")
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/bridge/br_netlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 2ff9706..e5ec470 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -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 },
 };
-- 
1.9.3

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

[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 <vyasevic@redhat.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/core/rtnetlink.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5a853f8..b9b7dfa 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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) {
-- 
1.9.3

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

[Jeff Kirsher]

On Wed, Nov 26, 2014 at 4:42 AM, Thomas Graf <tgraf@suug.ch> wrote:
> 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 <ajit.khaparde@emulex.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  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 <jeffrey.t.kirsher@intel.com>

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

[Jeff Kirsher]

On Wed, Nov 26, 2014 at 4:42 AM, Thomas Graf <tgraf@suug.ch> wrote:
> 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 <ajit.khaparde@emulex.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  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 <jeffrey.t.kirsher@intel.com>

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

[John Fastabend]

On 11/26/2014 04:42 AM, Thomas Graf wrote:
> 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 <ajit.khaparde@emulex.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>   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 <john.r.fastabend@intel.com>


-- 
John Fastabend         Intel Corporation

Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations

[John Fastabend]

On 11/26/2014 04:42 AM, Thomas Graf wrote:
> 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(-)
>

+Jiri

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

>
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 3e6eebd..7b11243 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -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,

-- 
John Fastabend         Intel Corporation

Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations

[Thomas Graf]

On 11/26/14 at 08:58am, John Fastabend wrote:
> On 11/26/2014 04:42 AM, Thomas Graf wrote:
> >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(-)
> >
> 
> +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.

Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations

[John Fastabend]

On 11/26/2014 09:06 AM, Thomas Graf wrote:
> On 11/26/14 at 08:58am, John Fastabend wrote:
>> On 11/26/2014 04:42 AM, Thomas Graf wrote:
>>> 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(-)
>>>
>>
>> +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.
>

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,

> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -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));
> +               }
>         }





-- 
John Fastabend         Intel Corporation

Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 26 Nov 2014 13:42:15 +0100

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

Series applied, thanks Thomas.

Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations

[Thomas Graf]

On 11/26/14 at 09:25am, John Fastabend wrote:
> >--- a/net/ipv4/devinet.c
> >+++ b/net/ipv4/devinet.c
> >@@ -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));
> >+               }

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

Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations

[John Fastabend]

On 11/26/2014 03:14 PM, Thomas Graf wrote:
> On 11/26/14 at 09:25am, John Fastabend wrote:
>>> --- a/net/ipv4/devinet.c
>>> +++ b/net/ipv4/devinet.c
>>> @@ -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));
>>> +               }
>
> Looked into this and found a validation function
> inet_validate_link_af(). It's split to keep the updates atomic.
>

Ah great thanks.

-- 
John Fastabend         Intel Corporation