[SRU,Bionic/Cosmic/Unstable,1/1] rtnetlink: fix rtnl_fdb_dump() for ndmsg header

Message ID 20181008200729.15392-2-mfo@canonical.com
State New
Headers show
Series
  • [SRU,Bionic/Cosmic/Unstable,1/1] rtnetlink: fix rtnl_fdb_dump() for ndmsg header
Related show

Commit Message

Mauricio Faria de Oliveira Oct. 8, 2018, 8:07 p.m.
BugLink: https://bugs.launchpad.net/bugs/1796748

Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
which is not always true -- 'struct ndmsg' is used by iproute2 ('ip neigh').

The problem is, the function bails out early if nlmsg_parse() fails, which
does occur for iproute2 usage of 'struct ndmsg' because the payload length
is shorter than the family header alone (as 'struct ifinfomsg' is assumed).

This breaks backward compatibility with userspace -- nothing is sent back.

Some examples with iproute2 and netlink library for go [1]:

 1) $ bridge fdb show
    33:33:00:00:00:01 dev ens3 self permanent
    01:00:5e:00:00:01 dev ens3 self permanent
    33:33:ff:15:98:30 dev ens3 self permanent

      This one works, as it uses 'struct ifinfomsg'.

      fdb_show() @ iproute2/bridge/fdb.c
        """
        .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
        ...
        if (rtnl_dump_request(&rth, RTM_GETNEIGH, [...]
        """

 2) $ ip --family bridge neigh
    RTNETLINK answers: Invalid argument
    Dump terminated

      This one fails, as it uses 'struct ndmsg'.

      do_show_or_flush() @ iproute2/ip/ipneigh.c
        """
        .n.nlmsg_type = RTM_GETNEIGH,
        .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)),
        """

 3) $ ./neighlist
    < no output >

      This one fails, as it uses 'struct ndmsg'-based.

      neighList() @ netlink/neigh_linux.go
        """
        req := h.newNetlinkRequest(unix.RTM_GETNEIGH, [...]
        msg := Ndmsg{
        """

The actual breakage was introduced by commit 0ff50e83b512 ("net: rtnetlink:
bail out from rtnl_fdb_dump() on parse error"), because nlmsg_parse() fails
if the payload length (with the _actual_ family header) is less than the
family header length alone (which is assumed, in parameter 'hdrlen').
This is true in the examples above with struct ndmsg, with size and payload
length shorter than struct ifinfomsg.

However, that commit just intends to fix something under the assumption the
family header is indeed an 'struct ifinfomsg' - by preventing access to the
payload as such (via 'ifm' pointer) if the payload length is not sufficient
to actually contain it.

The assumption was introduced by commit 5e6d24358799 ("bridge: netlink dump
interface at par with brctl"), to support iproute2's 'bridge fdb' command
(not 'ip neigh') which indeed uses 'struct ifinfomsg', thus is not broken.

So, in order to unbreak the 'struct ndmsg' family headers and still allow
'struct ifinfomsg' to continue to work, check for the known message sizes
used with 'struct ndmsg' in iproute2 (with zero or one attribute which is
not used in this function anyway) then do not parse the data as ifinfomsg.

Same examples with this patch applied (or revert/before the original fix):

    $ bridge fdb show
    33:33:00:00:00:01 dev ens3 self permanent
    01:00:5e:00:00:01 dev ens3 self permanent
    33:33:ff:15:98:30 dev ens3 self permanent

    $ ip --family bridge neigh
    dev ens3 lladdr 33:33:00:00:00:01 PERMANENT
    dev ens3 lladdr 01:00:5e:00:00:01 PERMANENT
    dev ens3 lladdr 33:33:ff:15:98:30 PERMANENT

    $ ./neighlist
    netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0x0, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
    netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x1, 0x0, 0x5e, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
    netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0xff, 0x15, 0x98, 0x30}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}

Tested on mainline (v4.19-rc6) and net-next (3bd09b05b068).

References:

[1] netlink library for go (test-case)
    https://github.com/vishvananda/netlink

    $ cat ~/go/src/neighlist/main.go
    package main
    import ("fmt"; "syscall"; "github.com/vishvananda/netlink")
    func main() {
        neighs, _ := netlink.NeighList(0, syscall.AF_BRIDGE)
        for _, neigh := range neighs { fmt.Printf("%#v\n", neigh) }
    }

    $ export GOPATH=~/go
    $ go get github.com/vishvananda/netlink
    $ go build neighlist
    $ ~/go/src/neighlist/neighlist

Thanks to David Ahern for suggestions to improve this patch.

Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error")
Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
Reported-by: Aidan Obley <aobley@pivotal.io>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Reviewed-by: David Ahern <dsahern@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry picked from commit bd961c9bc66497f0c63f4ba1d02900bb85078366)
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 net/core/rtnetlink.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

Comments

Stefan Bader Oct. 9, 2018, 6:58 a.m. | #1
On 08.10.2018 22:07, Mauricio Faria de Oliveira wrote:
> BugLink: https://bugs.launchpad.net/bugs/1796748
> 
> Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
> which is not always true -- 'struct ndmsg' is used by iproute2 ('ip neigh').
> 
> The problem is, the function bails out early if nlmsg_parse() fails, which
> does occur for iproute2 usage of 'struct ndmsg' because the payload length
> is shorter than the family header alone (as 'struct ifinfomsg' is assumed).
> 
> This breaks backward compatibility with userspace -- nothing is sent back.
> 
> Some examples with iproute2 and netlink library for go [1]:
> 
>  1) $ bridge fdb show
>     33:33:00:00:00:01 dev ens3 self permanent
>     01:00:5e:00:00:01 dev ens3 self permanent
>     33:33:ff:15:98:30 dev ens3 self permanent
> 
>       This one works, as it uses 'struct ifinfomsg'.
> 
>       fdb_show() @ iproute2/bridge/fdb.c
>         """
>         .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
>         ...
>         if (rtnl_dump_request(&rth, RTM_GETNEIGH, [...]
>         """
> 
>  2) $ ip --family bridge neigh
>     RTNETLINK answers: Invalid argument
>     Dump terminated
> 
>       This one fails, as it uses 'struct ndmsg'.
> 
>       do_show_or_flush() @ iproute2/ip/ipneigh.c
>         """
>         .n.nlmsg_type = RTM_GETNEIGH,
>         .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)),
>         """
> 
>  3) $ ./neighlist
>     < no output >
> 
>       This one fails, as it uses 'struct ndmsg'-based.
> 
>       neighList() @ netlink/neigh_linux.go
>         """
>         req := h.newNetlinkRequest(unix.RTM_GETNEIGH, [...]
>         msg := Ndmsg{
>         """
> 
> The actual breakage was introduced by commit 0ff50e83b512 ("net: rtnetlink:
> bail out from rtnl_fdb_dump() on parse error"), because nlmsg_parse() fails
> if the payload length (with the _actual_ family header) is less than the
> family header length alone (which is assumed, in parameter 'hdrlen').
> This is true in the examples above with struct ndmsg, with size and payload
> length shorter than struct ifinfomsg.
> 
> However, that commit just intends to fix something under the assumption the
> family header is indeed an 'struct ifinfomsg' - by preventing access to the
> payload as such (via 'ifm' pointer) if the payload length is not sufficient
> to actually contain it.
> 
> The assumption was introduced by commit 5e6d24358799 ("bridge: netlink dump
> interface at par with brctl"), to support iproute2's 'bridge fdb' command
> (not 'ip neigh') which indeed uses 'struct ifinfomsg', thus is not broken.
> 
> So, in order to unbreak the 'struct ndmsg' family headers and still allow
> 'struct ifinfomsg' to continue to work, check for the known message sizes
> used with 'struct ndmsg' in iproute2 (with zero or one attribute which is
> not used in this function anyway) then do not parse the data as ifinfomsg.
> 
> Same examples with this patch applied (or revert/before the original fix):
> 
>     $ bridge fdb show
>     33:33:00:00:00:01 dev ens3 self permanent
>     01:00:5e:00:00:01 dev ens3 self permanent
>     33:33:ff:15:98:30 dev ens3 self permanent
> 
>     $ ip --family bridge neigh
>     dev ens3 lladdr 33:33:00:00:00:01 PERMANENT
>     dev ens3 lladdr 01:00:5e:00:00:01 PERMANENT
>     dev ens3 lladdr 33:33:ff:15:98:30 PERMANENT
> 
>     $ ./neighlist
>     netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0x0, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
>     netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x1, 0x0, 0x5e, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
>     netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0xff, 0x15, 0x98, 0x30}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
> 
> Tested on mainline (v4.19-rc6) and net-next (3bd09b05b068).
> 
> References:
> 
> [1] netlink library for go (test-case)
>     https://github.com/vishvananda/netlink
> 
>     $ cat ~/go/src/neighlist/main.go
>     package main
>     import ("fmt"; "syscall"; "github.com/vishvananda/netlink")
>     func main() {
>         neighs, _ := netlink.NeighList(0, syscall.AF_BRIDGE)
>         for _, neigh := range neighs { fmt.Printf("%#v\n", neigh) }
>     }
> 
>     $ export GOPATH=~/go
>     $ go get github.com/vishvananda/netlink
>     $ go build neighlist
>     $ ~/go/src/neighlist/neighlist
> 
> Thanks to David Ahern for suggestions to improve this patch.
> 
> Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error")
> Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
> Reported-by: Aidan Obley <aobley@pivotal.io>
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> Reviewed-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry picked from commit bd961c9bc66497f0c63f4ba1d02900bb85078366)
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  net/core/rtnetlink.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index fececbf6840a..45e21c8b698b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3554,16 +3554,27 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	int err = 0;
>  	int fidx = 0;
>  
> -	err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> -			  IFLA_MAX, ifla_policy, NULL);
> -	if (err < 0) {
> -		return -EINVAL;
> -	} else if (err == 0) {
> -		if (tb[IFLA_MASTER])
> -			br_idx = nla_get_u32(tb[IFLA_MASTER]);
> -	}
> +	/* A hack to preserve kernel<->userspace interface.
> +	 * Before Linux v4.12 this code accepted ndmsg since iproute2 v3.3.0.
> +	 * However, ndmsg is shorter than ifinfomsg thus nlmsg_parse() bails.
> +	 * So, check for ndmsg with an optional u32 attribute (not used here).
> +	 * Fortunately these sizes don't conflict with the size of ifinfomsg
> +	 * with an optional attribute.
> +	 */
> +	if (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) &&
> +	    (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) +
> +	     nla_attr_size(sizeof(u32)))) {
> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> +				  IFLA_MAX, ifla_policy, NULL);
> +		if (err < 0) {
> +			return -EINVAL;
> +		} else if (err == 0) {
> +			if (tb[IFLA_MASTER])
> +				br_idx = nla_get_u32(tb[IFLA_MASTER]);
> +		}
>  
> -	brport_idx = ifm->ifi_index;
> +		brport_idx = ifm->ifi_index;
> +	}
>  
>  	if (br_idx) {
>  		br_dev = __dev_get_by_index(net, br_idx);
>
Thadeu Lima de Souza Cascardo Oct. 9, 2018, 1:27 p.m. | #2
Upstream fix, clean cherry-pick, very well tested.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Thadeu Lima de Souza Cascardo Oct. 10, 2018, 2:22 p.m. | #3
Applied to cosmic master-next branch.

Thanks.
Cascardo.
Mauricio Faria de Oliveira Oct. 15, 2018, 2:53 p.m. | #4
Hi,

On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira
<mfo@canonical.com> wrote:
> BugLink: https://bugs.launchpad.net/bugs/1796748

Just checking whether this will be applied on Bionic in this SRU cycle as well?

Thanks!
Kleber Souza Oct. 16, 2018, 7:46 a.m. | #5
On 10/15/18 16:53, Mauricio Faria de Oliveira wrote:
> Hi,
> 
> On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira
> <mfo@canonical.com> wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1796748
> 
> Just checking whether this will be applied on Bionic in this SRU cycle as well?
> 
> Thanks!
> 

Hi Mauricio,

The patch was reviewed and already has 2 ACK's so it's ready to be
applied for the next SRU cycle. This is the 3rd week of the current
cycle, so we'll start applying patches soon for the next which begins
next week.



Thanks,
Kleber

Patch

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index fececbf6840a..45e21c8b698b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3554,16 +3554,27 @@  static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	int err = 0;
 	int fidx = 0;
 
-	err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
-			  IFLA_MAX, ifla_policy, NULL);
-	if (err < 0) {
-		return -EINVAL;
-	} else if (err == 0) {
-		if (tb[IFLA_MASTER])
-			br_idx = nla_get_u32(tb[IFLA_MASTER]);
-	}
+	/* A hack to preserve kernel<->userspace interface.
+	 * Before Linux v4.12 this code accepted ndmsg since iproute2 v3.3.0.
+	 * However, ndmsg is shorter than ifinfomsg thus nlmsg_parse() bails.
+	 * So, check for ndmsg with an optional u32 attribute (not used here).
+	 * Fortunately these sizes don't conflict with the size of ifinfomsg
+	 * with an optional attribute.
+	 */
+	if (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) &&
+	    (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) +
+	     nla_attr_size(sizeof(u32)))) {
+		err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
+				  IFLA_MAX, ifla_policy, NULL);
+		if (err < 0) {
+			return -EINVAL;
+		} else if (err == 0) {
+			if (tb[IFLA_MASTER])
+				br_idx = nla_get_u32(tb[IFLA_MASTER]);
+		}
 
-	brport_idx = ifm->ifi_index;
+		brport_idx = ifm->ifi_index;
+	}
 
 	if (br_idx) {
 		br_dev = __dev_get_by_index(net, br_idx);