netdev - Re: [PATCH net-next] rtnetlink: add RTNH_REJECT

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:   Tue, 30 Nov 2021 11:28:32 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Alexander Mikhalitsyn <alexander.mikhalitsyn@...tuozzo.com>
Cc:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org,
        David Miller <davem@...emloft.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Ido Schimmel <idosch@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Roopa Prabhu <roopa@...dia.com>,
        Andrei Vagin <avagin@...il.com>,
        Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
        Alexander Mikhalitsyn <alexander@...alicyn.com>
Subject: Re: [PATCH net-next] rtnetlink: add RTNH_REJECT_MASK

On Tue, Nov 30, 2021 at 11:35:17AM +0300, Alexander Mikhalitsyn wrote:
> On Tue, 30 Nov 2021 09:59:25 +0200
> Ido Schimmel <idosch@...sch.org> wrote:
> > Looking at the patch again, what is the motivation to expose
> > RTNH_REJECT_MASK to user space? iproute2 already knows that it only
> > makes sense to set RTNH_F_ONLINK. Can't we just do:
> 
> Sorry, but that's not fully clear for me, why we should exclude RTNH_F_ONLINK?
> I thought that we should exclude RTNH_F_DEAD and RTNH_F_LINKDOWN just because
> kernel doesn't allow to set these flags.

I don't think we should exclude RTNH_F_ONLINK. I'm saying that it is the
only flag that it makes sense to send to the kernel in the ancillary
header of RTM_NEWROUTE messages. The rest of the RNTH_F_* flags are
either not used by the kernel or are only meant to be sent from the
kernel to user space. Due to omission, they are mistakenly allowed.

Therefore, I think that the only necessary patch is an iproute2 patch
that makes sure that during save/restore you are clearing all the
RTNH_F_* flags but RTNH_F_ONLINK.

BTW, looking at save_route() in iproute2, I think the patch only clears
these flags from the ancillary header, but not from 'struct rtnexthop'
that is nested in RTA_MULTIPATH for multipath routes. See this blog post
for depiction of the message:
http://codecave.cc/multipath-routing-in-linux-part-1.html

> 
> I'd also thought about another approach - "offload" this flags filtering
> problems to the kernel side for better iproute dump images compatibility.
> 
> Now we dump all routes using netlink message like this
> 	struct {
> 		struct nlmsghdr nlh;
> 		struct rtmsg rtm;
> 		char buf[128];
> 	} req = {
> 		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
> 		.nlh.nlmsg_type = RTM_GETROUTE,
> 		.nlh.nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST,
> ...
> 	};
> 
> But we can introduce some "special" flag like NLM_F_FILTERED_DUMP (or something like that)
> 	} req = {
> 		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
> 		.nlh.nlmsg_type = RTM_GETROUTE,
> 		.nlh.nlmsg_flags = NLM_F_FILTERED_DUMP | NLM_F_REQUEST,
> ...
> 	};
> 
> The idea here is that the kernel nows better which flags should be omitted from the dump
> (<=> which flags is prohibited to set directly from the userspace side).
> 
> But that change is more "global". WDYT about this?
> 
> I'm ready to implement any of the approaches with your kind advice.

Having the kernel filter RO flags upon RTM_GETROUTE with a new special
flag / attribute would be easiest to implement in iproute2 (especially
if my comment about RTA_MULTIPATH is correct), but it's a quite invasive
change that requires new uAPI.

Personally, I think that if something can be done in user space, then I
would do it in user space instead of adding new uAPI.