• Mauricio Faria de Oliveira's avatar
    rtnetlink: fix rtnl_fdb_dump() for ndmsg header · bd961c9b
    Mauricio Faria de Oliveira authored
    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 0ff50e83 ("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 5e6d2435 ("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 (3bd09b05).
    
    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: 0ff50e83 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error")
    Fixes: 5e6d2435 ("bridge: netlink dump interface at par with brctl")
    Reported-by: default avatarAidan Obley <aobley@pivotal.io>
    Signed-off-by: default avatarMauricio Faria de Oliveira <mfo@canonical.com>
    Reviewed-by: default avatarDavid Ahern <dsahern@gmail.com>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    bd961c9b
rtnetlink.c 116 KB