netdev - Re: [PATCH net-next] rtnetlink: fix rtnl_fdb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 1 Oct 2018 09:44:38 -0300
From:   Mauricio Faria de Oliveira <mfo@...onical.com>
To:     dsahern@...il.com
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter
 family headers

On Sun, Sep 30, 2018 at 10:06 PM David Ahern <dsahern@...il.com> wrote:
>
> On 9/28/18 1:35 PM, Mauricio Faria de Oliveira wrote:
> > Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
> > which is not always true.  For example, 'struct ndmsg' is used by iproute2
> > as well (in the 'ip neigh' command).
> >
> > 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 (different response) and
> > is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from
> >  rtnl_fdb_dump() on parse error").
> ...
>
> >
> > 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@...otal.io>
> > Signed-off-by: Mauricio Faria de Oliveira <mfo@...onical.com>
> > ---
> > P.S.: this may be 'net', but labeling as 'net-next' for possible relation to recent thread
> > [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request
> >
> >  net/core/rtnetlink.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index 60c928894a78..9695a27cc9b9 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -3744,16 +3744,17 @@ 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) {
> > +     /* The family header may _not_ be struct ifinfomsg
> > +      * (e.g., struct ndmsg).  Usage of the ifm pointer
> > +      * must check payload length (e.g., nlmsg_parse()).
> > +      */
> > +     if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> > +                     IFLA_MAX, ifla_policy, NULL) == 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);
> >
>

David, thanks for reviewing this.

> I suspect rtnl_fdb_dump is forever stuck with the ifinfomsg struct as
> the header if any kernel side filtering is to be done. [snip]

Why exactly?   I understand currently there may be little information
to distinguish family headers, but if it comes down to certain attributes
the function expects/uses, that can be checked if nlmsg_parse() is OK.

Otherwise, if it comes down to some struct field that is not common
between both structs, then.. well. Maybe something else/new.

> [...] As for the change
> above, I suggest something like this:
>
>         /* if header struct is ndmsg, no attributes can be appended */
>         if (nlmsg_len(nlh) != sizeof(struct ndmsg)) {
>                 current ifinfomsg based code
>         }
>

Hm, not sure -- ndmsg can be used with attributes too in iproute2, e.g., 'dev'.
In that case, the payload size is different/greater, and even makes
the length check pass:

$ ip --family bridge neigh
[  585.111034] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 12
RTNETLINK answers: Invalid argument
Dump terminated

$ ip --family bridge neigh show dev ens3
[  540.231443] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 20
[  540.234433] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 20
lladdr 33:33:00:00:00:01 PERMANENT
lladdr 01:00:5e:00:00:01 PERMANENT
lladdr 33:33:ff:e9:9d:60 PERMANENT

> We certainly do not want to ignore parse failures.

Well, if there are no attributes, that shouldn't be a problem,
but unfortunately that's what happens in this case :- (

Now, given the example above makes the attribute parsing pass,
that allows the payload interpretation as ifm struct to be used.
Fortunately this field is commonly aligned on both structs,
and is not used in ndmsg per iproute2 ipneigh.c do_show_or_flush().
But this might pose problems in the future if things change.

If you could explain your thoughts a bit more about it, it would be
really great.

Thanks again,

-- 
Mauricio Faria de Oliveira