Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 2. Oct 2019, at 23:02, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> On 02/10/2019 10:50, Wieczorkiewicz, Pawel wrote:
>>>>>> I've taken, as a simple example,
>>>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has 
>>>>>> generated
>>>>>> code (in a non-debug build). Hence either I'm mis-remembering what we 
>>>>>> want
>>>>>> things to look like, or there's more to it than code generation simply 
>>>>>> being
>>>>>> "not correct".
>>>>>> 
>>>>> This example demonstrates the problem, and actually throws a further
>>>>> spanner in the works of how make this safe, which hadn't occurred to me
>>>>> before.
>>>>> 
>>>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>>>> will look something like this:
>>>>> 
>>>>> call p2m_mem_access_sanity_check
>>>>>     ...
>>>>>     lfence
>>>>>     ...
>>>>>     ret   
>>>>> cmp $0, %eax
>>>>> jne ...
>>>>> 
>>>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>>> 
>>>>> call p2m_mem_access_sanity_check
>>>>>     ...
>>>>>     ret
>>>>> cmp $0, %eax
>>>>> jne 1f
>>>>> lfence
>>>>> ...
>>>>> 1: lfence
>>>>> …
>>>>> 
>>>>> 
> 
> Answering out of order, because I think this will make things clearer.
> 
>> But the hardening wasn’t about spectre v1, but about cache-load gadgets?
> 
> Ultimately, yes - the goal is cache load gadgets.
> 
> Cache load gadgets are any memory read, where the attacker can influence the 
> position of the load.  The easy case to think about is the first half of a 
> Spectre v1 gadget (i.e. the first memory load), but a second common case is a 
> simple memory read with the wrong base pointer (as demonstrated clearly by 
> SpectreGS and CVE-2019-1125).
> 

Yes, that’s right.

> A 3rd case, which is actually the root of this discovery, is speculative type 
> confusion where the processor executes code expecting to use 
> {d,v}->arch.{pv,hvm}.$FOO, but is interpreting the data with the types of the 
> other union.  For people familiar with Speculative Store Bypass gadgets, this 
> is the same kind of thing as the integer/pointer confusion in that case.
> 

Yes, that’s right again. There is also a few other cases like abusing switch 
jump tables etc.
But, I would like to focus on the first half of a Spectre v1 gadgets.

> The only viable fix for these is to avoid entering the basic block with the 
> vulnerable code pattern in the first place.  I.e. "fixing" Spectre v1.
> 

I think that’s not the only viable fix option. But, that depends on the basic 
block construction.
There certainly are basic block where this is the only viable fix. But there 
are also others.

Example 1:

mov (REG1), REG2
cmp $0x0,(REG3)
jne 1
mov (REG2), REG4 # Gadget load
1:
...

In the above case a lfence does not need to protect the branch speculation to 
kill off the cache-load gadget.
The lfence before the cmp instruction would make sure the REG2 holds an actual 
value.

Example 2: (probably a better one)

mov (REG1), REG2
cmp $0x0,REG2
jne 1
mov (REG3), REG4 # Gadget load
1:
...

When putting lfence before the cmp instruction, we limit the chances of 
speculation over the branch.
Same applies to this:

call is_allowed
    mov (REG1), REG2
    ….   
    mov REG2, RAX
    ret
cmp $0x0,RAX
jne 1
mov (REG2), REG4 # Gadget load
1:
...

Putting lfence before return from the function, so as to have the value for the 
cmp instruction fixed would be also good enough.
(I agree that might not be perfect, but just good enough…)

And finally:
Example 3:

cmp $0x0,(REG1)
jne 1
mov (REG2), REG3 # Gadget load
1:
…

Here indeed not much can be done and we have to put lfences the spectre v1 way.


>> Does it mean the CPU speculates over a direct call (assuming no #PF etc) and
>> assumes some default return value to be used?
>> 
> 
> That wasn't the point I was trying to make, although Intel CPUs will 
> speculatively execute the instructions following an indirect call/jmp while 
> the frontend works out where to fetch from next.
> 

Yes, indirect call/jmp instruction could speculate, but that’s out of scope for 
this discussion.

> The point was that, to avoid entering the wrong basic block, the lfence must 
> be later in the instruction stream than the conditional jump.  The frontend 
> has to follow the attacker controlled jump, then serialise itself to figure 
> out if it went wrong.
> 

Yes, that’s true. But, this is to avoid entering the wrong basic block. As I 
mentioned earlier that might not be the only option.

> Serialising first, then following the attacker controlled jump still leaves a 
> speculation window, which can execute the gadget.
> 

Yes, that’s also true. Such protection against entering a basic block does not 
guarantee anything.
One could argue that it limits the speculation window though. And, it makes it 
harder to orchestrate the attack.
But, I agree that this is a false sense of security.

>> If not, maybe we should be more concerned about the value the cache-loading
>> gadget speculates with, rather than the sheer speculation over the branch.
>> 
> 
> In a mythical world where a complete solution existed, channels other than 
> the data cache want considering.  There is some interesting work with 
> instruction cache and TLB timing analysis, and these are far harder to close.
> 
> Given the SpectreGS case of a bad base pointer, rather than a bad offset, a 
> non-spectre-v1 fix would have to clamp every register making up part of a 
> memory operand.
> 

Yes, but only every register that could be influenced by an untrusted source.
That limits the overall number, but has its own challenges (taint analysis is 
hard as it is anyway…).

> This is the approach which the Clang/LLVM Speculative Load Hardening feature 
> goes for, and comes with a 30% perf hit or so.  Furthermore, the current 
> design of SLH is specific to userspace, and there is development work needed 
> to make it safe for kernel mode code.
> 

Yes, I have spent some time with it. A very nice stuff but definitely still in 
the making.

> Specifically, the muxing of the speculation token against registers needs to 
> turn from &= token to |= token, and the sense of the token being 0 or -1 
> needs to reverse, because of kernel code operating in the top of the address 
> space, rather than the bottom.  There is a further complication given signed 
> displacement.  For kernel code, that can still speculatively wrap around into 
> userspace, and SMAP (on meltdown-vulnerable parts) won't even halt 
> speculation in this case.
> 

Yeah, I agree. That brings memories of XSA212. Good ol’ times ;-).

> Further further more, for Xen, that would still move incorrect speculative 
> memory accesses into PV guest kernel controlled space, rather than Xen 
> controlled space.
> 
>> 
>>> To protect against speculatively executing the wrong basic block, the
>>> pipeline must execute the conditional jump first, *then* hit an lfence
>>> to serialise the instruction stream and revector in the case of
>>> incorrect speculation.
>>> 
>> That’s true, but there are also 2 aspects worth mentioning:
>> 1) Example:
>> 
>> jne 1
>> PRIVILEGED
>> 1:
>> ALWAYS_SAFE
>> 
>> We do not necessarily have to cover the 1: path with an lfence?
>> We could allow speculation there, as it is harmless.
>> 
> 
> I agree, but how do you express that using evaluate_nospec()?
> 
> There is no information tying what is privileged and what is safe, to the 
> condition for entering the basic block.
> 

Yes, I think we all agree that trying to solve the general case with such 
mitigations might be infeasible and fragile.
But, despite that manual application of the mitigations demands a lot of effort 
and caution, I would argue that there
are places in code where it makes sense as a best-effort solution "for now".


>> 2) cache-load protection
>> 
>> It might be ok to speculate over a conditional branch, when we can
>> guarantee that the value to be used in a dereference is not out-of-bound.
>> 
> 
> I agree (in principle, because the guarantee is implausible to make in 
> general case) but…
> 

Yes, I of course agree that there is absolutely no guarantee for general case.
And if there was a general case solution I would strongly recommend to use that 
instead.

>> In that case an lfence is used to latch the value in the register. We can
>> still speculate over the branch and reach the dereference, but with a sane 
>> value.
>> 
> 
> ... this is reasoning about the wrong basic block.  This analogy is:
> 
> cmp ... #1
> jcc 1f
>     ALWAYS_SAFE
>     lfence ("optimised" from the cmp #2 if statement)
>     cmp ... #2
>     jcc 1f
>     PRIVILEGED
> 1:
> ALWAYS_SAFE
> 

Well, the above code may be one or all of my examples (1,2, 3) from above.

> This is saying that the spilled lfence from cmp2 protects PRIVLEGED because 
> it might reduce the speculative variability in registers.  There are probably 
> code sequences where that would be true, but there are plenty of others where 
> it would not be true.
> 

Totally agree with you here. I provided more thoughts for the examples above.

> This is because it is protecting cmp1's basic block (or at least, part of 
> it), not cmp2's.  It will protect against an attack which requires poisoning 
> of both cmp1 and cmp2 to be function, but this is orders of magnitude harder 
> for the attacker to detect and arrange for, than an attack which simply 
> involves poisoning cmp2 to enter PRIVILEGED with an architecturally-correct 
> register state intended for ALWAYS_SAFE.
> 

Yes, that’s all true.
But, the actual incarnation of the above code, could be actually good enough 
iff the PRIVILEGED part does not do any memory dereferences with uncertain, 
attacker-influenced values (I have example 2 from above in mind here). 

>> I agree that lfence might not give us 100% security in every potential case,
>> but that is what "hardening" gives you...
>> 
> 
> This being a best-effort approach is fine.  There is no way it could ever be 
> called complete with a straight face.
> 

Totally agree again. It is definitely not even close to being complete. Better 
than nothing though.

> If everything had been done using block_speculation(), that would also be ok. 
>  There is no way the optimiser can break the safety there, because it cannot 
> reposition the lfence instruction relative to any memory references (although 
> the subtleties of it being able to be repositioned relative to non-memory 
> accesses still make it hard to reason about in general).
> 

Yes, that is still a band-aid mitigation, killing off most of the (more or 
less) obvious cache-load gadgets that manual effort can find.
General case is still open and I personally believe, that only compiler or 
silicon fixes could try to provide a complete, general enough
solution.

> The problem is that, at the moment, the optimiser is undermining the safety 
> which is attempting to be inserted by the use of evaluate_nospec().
> 

Yes. I have not see that myself (with GCC7), but I can imagine that could be 
easily the case.

> We have code which appears to be safe at the C level, and isn't.  A false 
> sense of security is arguably worse than no security.
> 

Yes, I agree with such reasoning (if we are speaking of general case with 
evaluate_nospec()).

> The random sprinkling of lfences will reduce the amount of speculation, but 
> this is of very little use when it is only actually protecting against 
> certain more-complicated, and therefore rarer, gadgets but leaves the common 
> gadgets still exploitable.
> 

I agree in principle, but I do not think that this is the case with the patches 
in question (at least not with majority of the changes there). 

> ~Andrew

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel