ACK: [SRU][Bionic/Cosmic/Unstable][PATCH 1/1] rtnetlink: fix rtnl_fdb_dump() for ndmsg header
Stefan Bader
stefan.bader at canonical.com
Tue Oct 9 06:58:28 UTC 2018
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 at pivotal.io>
> Signed-off-by: Mauricio Faria de Oliveira <mfo at canonical.com>
> Reviewed-by: David Ahern <dsahern at gmail.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit bd961c9bc66497f0c63f4ba1d02900bb85078366)
> Signed-off-by: Mauricio Faria de Oliveira <mfo at canonical.com>
Acked-by: Stefan Bader <stefan.bader at 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);
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20181009/d3e545d4/attachment.sig>
More information about the kernel-team
mailing list