[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
Mauricio Faria de Oliveira Oct. 16, 2018, 12:49 p.m. | #6
Hi Kleber,

Thanks for confirming!  I guess I'm interpreting the dates in 'A new
SRU cycle started' announces incorrectly...

> 17-Oct   Last day for kernel commits for this cycle.
> 22-Oct - 27-Oct   Kernel prep week.

The 'last day for commits' is for patches/commits to be sent to the
mailing list or to be applied to git trees?
I guess my confusion is whether to interpret what is sent as
_patches_, and what is applied in git as _commits_.

Kind regards,
Thanks!

On Tue, Oct 16, 2018 at 4:46 AM Kleber Souza <kleber.souza@canonical.com> wrote:
>
> 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
Kleber Souza Oct. 16, 2018, 1:05 p.m. | #7
On 10/16/18 14:49, Mauricio Faria de Oliveira wrote:
> Hi Kleber,
> 
> Thanks for confirming!  I guess I'm interpreting the dates in 'A new
> SRU cycle started' announces incorrectly...
> 
>> 17-Oct   Last day for kernel commits for this cycle.
>> 22-Oct - 27-Oct   Kernel prep week.
> 
> The 'last day for commits' is for patches/commits to be sent to the
> mailing list or to be applied to git trees?
> I guess my confusion is whether to interpret what is sent as
> _patches_, and what is applied in git as _commits_.

The "last day" is basically the deadline for having patches at least
reviewed and ACK'ed on the mailing-list. Depending on the load of the
team we might start applying the patches only later in the week, but if
they are ACK'ed they are guaranteed to be picked up.

Having that in mind, we don't guarantee though that if a patch series
sent before the deadline will be reviewed and applied in time. An
example would be patches that introduce a huge amount of changes or
touch a sensitive part of the kernel, which could have a high potential
for regressions. If we evaluate that more care is needed on the review
of those patches, we might decide to postpone them to the next cycle.

So the recommendation is to send the patches as soon as possible and nag
someone if they didn't get enough attention when the deadline is getting
close :-).


Thanks,
Kleber

> 
> Kind regards,
> Thanks!
> 
> On Tue, Oct 16, 2018 at 4:46 AM Kleber Souza <kleber.souza@canonical.com> wrote:
>>
>> 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
> 
> 
>
Mauricio Faria de Oliveira Oct. 16, 2018, 3:42 p.m. | #8
On Tue, Oct 16, 2018 at 10:05 AM Kleber Souza
<kleber.souza@canonical.com> wrote:
>
> On 10/16/18 14:49, Mauricio Faria de Oliveira wrote:
> > Hi Kleber,
> >
> > Thanks for confirming!  I guess I'm interpreting the dates in 'A new
> > SRU cycle started' announces incorrectly...
> >
> >> 17-Oct   Last day for kernel commits for this cycle.
> >> 22-Oct - 27-Oct   Kernel prep week.
> >
> > The 'last day for commits' is for patches/commits to be sent to the
> > mailing list or to be applied to git trees?
> > I guess my confusion is whether to interpret what is sent as
> > _patches_, and what is applied in git as _commits_.
>
> The "last day" is basically the deadline for having patches at least
> reviewed and ACK'ed on the mailing-list. Depending on the load of the
> team we might start applying the patches only later in the week, but if
> they are ACK'ed they are guaranteed to be picked up.

Ah, now I got it.  Last day for patches to get enough ACKs. :- ) Noted!

> Having that in mind, we don't guarantee though that if a patch series
> sent before the deadline will be reviewed and applied in time. An
> example would be patches that introduce a huge amount of changes or
> touch a sensitive part of the kernel, which could have a high potential
> for regressions. If we evaluate that more care is needed on the review
> of those patches, we might decide to postpone them to the next cycle.

Sure, certainly understandable.

> So the recommendation is to send the patches as soon as possible and nag
> someone if they didn't get enough attention when the deadline is getting
> close :-).

Ok, right.

Thank you very much for the detailed explanation.

>
>
> Thanks,
> Kleber
>
> >
> > Kind regards,
> > Thanks!
> >
> > On Tue, Oct 16, 2018 at 4:46 AM Kleber Souza <kleber.souza@canonical.com> wrote:
> >>
> >> 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);