Re: [PATCH v2 5/7] rust: file: add `Kuid` wrapper

[Kees Cook]

On Fri, Dec 08, 2023 at 05:57:02PM +0100, Peter Zijlstra wrote:
> On Fri, Dec 08, 2023 at 05:31:59PM +0100, Miguel Ojeda wrote:
> > On Wed, Dec 6, 2023 at 2:41 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > >
> > > Anywhoo, the longer a function is, the harder it becomes, since you need
> > > to deal with everything a function does and consider the specuation
> > > window length. So trivial functions like the above that do an immediate
> > > dereference and are (and must be) a valid indirect target (because
> > > EXPORT) are ideal.
> > 
> > We discussed this in our weekly meeting, and we would like to ask a
> > few questions:
> > 
> >   - Could you please describe an example attack that you are thinking
> > of? (i.e. a "full" attack, rather than just Spectre itself). For
> > instance, would it rely on other vulnerabilities?
> 
> There's a fairly large amount of that on github, google spectre poc and
> stuff like that.

tl;dr: I don't think the introduction of speculation gadgets is a
sufficient reason to block Rust interfaces like this.

Long version:

I think the question here is "what is the threat model?" If I break down
the objection, I understand it as:

1) The trivial wrappers-of-inlines are speculation gadgets.
2) They're exported, so callable by anything.

If the threat model is "something can call these to trigger
speculation", I think this is pretty strongly mitigated already;

1) These aren't syscall definitions, so their "reachability" is pretty
limited. In fact, they're already going to be used in places, logically,
where the inline would be used, so the speculation window is going to be
same (or longer, given the addition of the direct call and return).

2) If an attacker is in a position to directly call these helpers,
they're not going to use them: if an attacker already has arbitrary
execution, they're not going to bother with speculation.

Fundamentally I don't see added risk here. From the security hardening
perspective we have two goals: kill bug classes and block exploitation
techniques, and the former is a much more powerful defensive strategy
since without the bugs, there's no chance to perform an exploit.

In general, I think we should prioritize bug class elimination over
exploit technique foiling. In this case, we're adding a potential weakness
to the image of the kernel of fairly limited scope in support of stronger
bug elimination goals.

Even if we look at the prerequisites for mounting an attack here, we've
already got things in place to help mitigate arbitrary code execution
(KCFI, BTI, etc). Nothing is perfect, but speculation gadgets are
pretty far down on the list of concerns, IMO. We have no real x86 ROP
defense right now in the kernel, so that's a much lower hanging fruit
for attackers.

As another comparison, on x86 there are so many direct execution gadgets
present in middle-of-instruction code patterns that worrying about a
speculation gadget seems silly to me.

> [...]
> The thing at hand was just me eyeballing it.

I can understand the point you and Greg have both expressed here: "this
is known to be an anti-pattern, we need to do something else". I generally
agree with this, but in this case, I don't think it's the right call. This
is an area we'll naturally see improvement from on the Rust side since
these calls are a _performance_ concern too, so it's not like this will be
"forgotten" about. But blocking it until there is a complete and perfect
solution feels like we're letting perfect be the enemy of good.

All of our development is evolutionary, so I think we'd be in a much
better position to take these (currently ugly) work-arounds (visible
only in Rust builds) so that we gain the ability to evolve towards more
memory safe code.

-Kees

-- 
Kees Cook