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
| ||
|
Date: Wed, 9 Mar 2022 20:49:16 +0100 From: Ilya Maximets <i.maximets@....org> To: Jakub Kicinski <kuba@...nel.org>, Roi Dayan <roid@...dia.com> Cc: i.maximets@....org, "David S. Miller" <davem@...emloft.net>, Pravin B Shelar <pshelar@....org>, Toms Atteka <cpp.code.lv@...il.com>, netdev@...r.kernel.org, dev@...nvswitch.org, linux-kernel@...r.kernel.org, Johannes Berg <johannes@...solutions.net>, Aaron Conole <aconole@...hat.com> Subject: Re: [PATCH net-next] net: openvswitch: fix uAPI incompatibility with existing user space On 3/9/22 18:22, Ilya Maximets wrote: > On 3/9/22 16:56, Ilya Maximets wrote: >> Few years ago OVS user space made a strange choice in the commit [1] >> to define types only valid for the user space inside the copy of a >> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >> added later. >> >> This leads to the inevitable clash between user space and kernel types >> when the kernel uAPI is extended. The issue was unveiled with the >> addition of a new type for IPv6 extension header in kernel uAPI. >> >> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >> older user space application, application tries to parse it as >> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >> every IPv6 packet that goes to the user space, IPv6 support is fully >> broken. >> >> Fixing that by bringing these user space attributes to the kernel >> uAPI to avoid the clash. Strictly speaking this is not the problem >> of the kernel uAPI, but changing it is the only way to avoid breakage >> of the older user space applications at this point. >> >> These 2 types are explicitly rejected now since they should not be >> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >> out from the '#ifdef __KERNEL__' as there is no good reason to hide >> it from the userspace. And it's also explicitly rejected now, because >> it's for in-kernel use only. >> >> Comments with warnings were added to avoid the problem coming back. >> >> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >> >> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >> Reported-by: Roi Dayan <roid@...dia.com> >> Signed-off-by: Ilya Maximets <i.maximets@....org> >> --- >> >> Roi, could you please test this change on your setup? >> >> I didn't run system tests myself yet, setting up environment at the moment. > > OK. I set up my own environment and the patch doesn't seem to work. > Investigating. I figured that out. The problem is that OVS_KEY_ATTR_IPV6_EXTHDRS == 32. That causes (1 << type) to overflow during the parsing and enable incorrect bit in the mask of seen attributes. The following additional change fixes the problem: diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index c9c49e5cd67f..5176f6ccac8e 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -489,7 +489,7 @@ static int __parse_flow_nlattrs(const struct nlattr *attr, return -EINVAL; } - if (attrs & (1 << type)) { + if (attrs & (1ULL << type)) { OVS_NLERR(log, "Duplicate key (type %d).", type); return -EINVAL; } @@ -502,7 +502,7 @@ static int __parse_flow_nlattrs(const struct nlattr *attr, } if (!nz || !is_all_zero(nla_data(nla), nla_len(nla))) { - attrs |= 1 << type; + attrs |= 1ULL << type; a[type] = nla; } } --- I'll run few more checks to be sure and send v2 with above change applied. We may also want to have a cosmetic change that converts all (1 << OVS_*) to (1ULL << OVS_*) even for attributes < 32 as a follow up. Current code looks a bit inconsistent. Best regards, Ilya Maximets.
Powered by blists - more mailing lists