netdev - Re: [PATCH ipsec] xfrm: migrate: work around 0 if

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]

Message-ID: <20241017105251.GA12005@breakpoint.cc>
Date: Thu, 17 Oct 2024 12:52:51 +0200
From: Florian Westphal <fw@...len.de>
To: Maciej Żenczykowski <maze@...gle.com>
Cc: Florian Westphal <fw@...len.de>, netdev@...r.kernel.org,
	Nathan Harold <nharold@...gle.com>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Yan Yan <evitayan@...gle.com>
Subject: Re: [PATCH ipsec] xfrm: migrate: work around 0 if_id on migrate

Maciej Żenczykowski <maze@...gle.com> wrote:
> > +found:
> >         /* Stage 2 - find and update state(s) */
> >         for (i = 0, mp = m; i < num_migrate; i++, mp++) {
> >                 if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
> > --
> > 2.45.2
> >
> 
> Q: Considering the performance impact... would it make sense to hide
> this behind a sysctl or a kconfig?

Kconfig?  I don't think so, all distros except Android would turn it on.

> Yan Yan: Also, while I know we found this issue in Android... do we
> actually need the fix?  Wasn't the adjustment to netd sufficient?
> Android <16 doesn't support >6.6 anyway, and Android 16 should already
> have the netd fix...

... seems you already fixed this, so I suspect this slowpath won't ever
run in your case.

Following relevant cases exist:
1. Userspace asks to migrate existing policy, provides if_id > 0.
   -> slowpath is elided.

2. Userspace asks to migrate existing policy, the policy is NOT for
   xfrm_interface, -> slowpath is also elided because first attempt
   finds the if_id 0 policy.

3. Like 1, but userspace does not set the if_id.
   -> slowpath runs, BUT without it migration would not work.

4. Like 2, but the policy doesn't exist.
   -> slowpath runs and slows things down for no reason.

For 1 and 2 even sysctl knob is irrelevant.

For 3, sysctl knob is *technically* irrelevant, either migrate is
broken (sysctl off) or its on and policy migrate will work.
This also hints we'd have to turn such sysctl on by default...

For 4, sysctl could be used to disable/avoid such slowdown.
But I'm not sure this is a relevant scenario in practice, aside
from fuzzers, AND it breaks 3) again if its off.

So I don't see a need to provide a config knob or a sysctl
that would have to be on by default...

If you think a Kconfig knob makes sense for Android sake I can respin
with such a knob, but I think I'd have to make it default-y.