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 12:38:36 -0300
From:   Mauricio Faria de Oliveira <mfo@...onical.com>
To:     David Ahern <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 Mon, Oct 1, 2018 at 12:01 PM David Ahern <dsahern@...il.com> wrote:
>
> On 10/1/18 6:44 AM, Mauricio Faria de Oliveira wrote:
> >> 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.
>
> struct ifinfomsg is 16 bytes; ndmsg is 12 bytes. The difference is
> ifi_change in the ifinfomsg. If you don't know which header is sent, how
> can you reliably parse -- and believe -- the result of the parsing?
>
> Yes, iproute2 0's out the structs. So does FRR. But the general argument
> is that userspace may not and the kernel has to this point happily
> ignored fields it was not using.

Right, I see the point of the unknown header type, and wanted to understand
whether you had more reasons in mind, as I imagined the particular
users/attributes
involved in this function could have provided an way out.

*But*, as you pointed out, the general case is that userspace can do anything.
So, now I see -- the assumption for particular cases is not an option. Thanks.

> The short of it is that ifi_change may be non-0 and should not be relied
> on. That's the theory anyways ...

And/or ndm_ifindex, which in the ifinfomsg type cast, gives brport_idx
a non-zero value
I don't yet know if it's equivalent, but if not, that's a problem.
(i.e., specify a non-brport thing, but it's processed as such)

> <snip>
> Perhaps there is a work around. IFLA_MASTER is the only supported
> attribute that can be appended, and it is sent as a u32. Then the
> rtnl_fdb_dump function has 4 legitimate cases:
>
> 1. ndmsg = size 12 bytes
> 2. ndmsg + MASTER = 20 ?
> 3. ifinfomsg = size 16 bytes
> 4. ifinfomsg + MASTER = 24 ?
>
> 'ip neigh show' could send NDA_IFINDEX as an additional attribute. That
> is my mistake. I should have set ndm_ifindex rather than using the
> attribute, but that ship sailed 3 years ago. Anyways, that case too
> might be uniquely detected. The size checks have been used in other
> places, so should be ok here too.

Ok, thanks for your suggestions.
I'll do some research/learning on them, and give it a try for a v2.

> At this point the use of ifinfomsg for dumps has created a mess
> extending kernel side filtering. The point of the PROPER_HDR patch set
> is to give a point in time across all dump functions where the kernel
> and userspace can reliably communicate about what is wanted.

Nice. I guess for this particular problem/thread, we'll won't be able to
use that one, since existing/older userspace that doesn't know about
it should be fixed for.. but definitely a great thing that should help in
this general problem.

Thank you,

-- 
Mauricio Faria de Oliveira