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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 May 2020 11:48:01 +0300 From: Nikolay Aleksandrov <nikolay@...ulusnetworks.com> To: David Ahern <dsahern@...il.com>, Roopa Prabhu <roopa@...ulusnetworks.com>, davem@...emloft.net Cc: netdev@...r.kernel.org, jiri@...lanox.com, idosch@...lanox.com, petrm@...lanox.com Subject: Re: [PATCH net-next 1/6] nexthop: dereference nh only once in nexthop_select_path On 19/05/2020 06:25, David Ahern wrote: > On 5/18/20 8:14 PM, Roopa Prabhu wrote: >> From: Nikolay Aleksandrov <nikolay@...ulusnetworks.com> >> >> the ->nh pointer might become suddenly null while we're selecting the >> path and we may dereference it. Dereference it only once in the >> beginning and use that if it's not null, we rely on the refcounting and >> rcu to protect against use-after-free. (This is needed for later >> vxlan patches that exposes the problem) >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com> >> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com> >> --- >> net/ipv4/nexthop.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> > > Reviewed-by: David Ahern <dsahern@...il.com> > > Should this be a bug fix? Any chance the route path can hit it? > That was fast, I didn't expect to see it on upstream so quickly. :) So I haven't had time to inspect it in detail, but it did seem to me that it should be possible to hit from the route path. When I tried running a few basic tests to make it happen I couldn't mainly due to the fib flush done at nexthop removal, but I still believe with the right timing one could hit it. In fact I'd go 1 step further and add a null check from the return of nexthop_select_path() in the helpers which dereference the value it returns like: nexthop_path_fib6_result() and nexthop_path_fib_result() The reason is that the .nh ptr is set and read without any sync primitives so in theory it can become null at any point (being cleared on nh group removal), and also the nh count in a group (num_nh), when it becomes == 0 while destroying a nh group and we hit it then in nexthop_select_path() rc would remain == NULL and we'll deref a null ptr. We did see the above with the vxlan code due to it missing the equivalent of a fib flush (or rather it being more relaxed), but I haven't had time to see how feasible it is to hit it via the route path yet.
Powered by blists - more mailing lists