Re: [PATCH 0/7] Mitigation against unsafe data speculation (CVE-2017-5753)

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 10/07/18 16:42, Jeff Law wrote:
> On 07/10/2018 02:49 AM, Richard Earnshaw (lists) wrote:
>> On 10/07/18 00:13, Jeff Law wrote:
>>> On 07/09/2018 10:38 AM, Richard Earnshaw wrote:
>>>>
>>>> To address all of the above, these patches adopt a new approach, based
>>>> in part on a posting by Chandler Carruth to the LLVM developers list
>>>> (https://lists.llvm.org/pipermail/llvm-dev/2018-March/122085.html),
>>>> but which we have extended to deal with inter-function speculation.
>>>> The patches divide the problem into two halves.
>>> We're essentially turning the control dependency into a value that we
>>> can then use to munge the pointer or the resultant data.
>>>
>>>>
>>>> The first half is some target-specific code to track the speculation
>>>> condition through the generated code to provide an internal variable
>>>> which can tell us whether or not the CPU's control flow speculation
>>>> matches the data flow calculations.  The idea is that the internal
>>>> variable starts with the value TRUE and if the CPU's control flow
>>>> speculation ever causes a jump to the wrong block of code the variable
>>>> becomes false until such time as the incorrect control flow
>>>> speculation gets unwound.
>>> Right.
>>>
>>> So one of the things that comes immediately to mind is you have to run
>>> this early enough that you can still get to all the control flow and
>>> build your predicates.  Otherwise you have do undo stuff like
>>> conditional move generation.
>>
>> No, the opposite, in fact.  We want to run this very late, at least on
>> Arm systems (AArch64 or AArch32).  Conditional move instructions are
>> fine - they're data-flow operations, not control flow (in fact, that's
>> exactly what the control flow tracker instructions are).  By running it
>> late we avoid disrupting any of the earlier optimization passes as well.
> Ack.  I looked at the aarch64 implementation after sending my message
> and it clearly runs very late.
> 
> I haven't convinced myself that all the work generic parts of the
> compiler to rewrite and eliminate conditionals is safe.  But even if it
> isn't, you're probably getting enough coverage to drastically reduce the
> attack surface.  I'm going to have to think about the early
> transformations we make and how they interact here harder.  But I think
> the general approach can dramatically reduce the attack surface.

My argument here would be that we are concerned about speculation that
the CPU does with the generated program.  We're not particularly
bothered about the abstract machine description it's based upon.  As
long as the earlier transforms lead to a valid translation (it hasn't
removed a necessary bounds check) then running late is fine.

I can't currently conceive a situation where the compiler would be able
to remove a /necessary/ bounds check that could lead to unsafe
speculation later on.  A redundant bounds check removal shouldn't be a
problem as the non-redundant check should remain and that will still get
tracking code added.

> 
> With running very late, as you noted, the big concern is edge
> insertions.  I'm going to have to re-familiarize myself with all the
> rules there :-)    I did note you stumbled on some of the issues in that
> space (what to do with calls that throw exceptions).
> 
> Placement before the final bbro pass probably avoids a lot of pain.  So
> the basic placement seems reasonable.  And again, if we're missing
> something due to the effects of earlier passes, I still think you're
> reducing the attack surface in a meaningful way.
> 
> 
> 
>>
>>>
>>> On the flip side, the earlier you do this mitigation, the more you have
>>> to worry about what the optimizers are going to do to the code later in
>>> the pipeline.  It's almost guaranteed a naive implementation is going to
>>> muck this up since we can propagate the state of the condition into the
>>> arms which will make the predicate state a compile time constant.
>>>
>>> In fact this seems to be running into the area of pointer providence and
>>> some discussions we had around atomic a few years back.
>>>
>>> I also wonder if this could be combined with taint analysis to produce a
>>> much lower overhead solution in cases were developers have done analysis
>>> and know what objects are potentially under attacker control.  So
>>> instead of analyzing everything, we can have a much narrower focus.
>>
>> Automatic application of the tracker to vulnerable variables would be
>> nice, but I haven't attempted to go there yet: at present I still rely
>> on the user to annotate code with the new intrinsic.
> ACK.  My sense is we are going to want taint analysis.  I think it'd be
> useful here and in other contexts.  However, I don't think it
> necessarily needs to be a requirement to go forward.
> 
> I'm going to review the atomic discussion we had a while back with the
> kernel folks as well as some pointer providence discussions I've had
> with Martin S.  I can't put my finger on it yet, but I still have the
> sense there's some interactions here we want to at least be aware of.
> 
>>
>> That doesn't mean that we couldn't extend the overall approach later to
>> include automatic tracking.
> Absolutely.
> 
>>
>>>
>>> The pointer munging could well run afoul of alias analysis engines that
>>> don't expect to be seeing those kind of operations.
>>
>> I think the pass runs late enough that it isn't a problem.
> Yea, I think you're right.
> 
> 
>>
>>>
>>> Anyway, just some initial high level thoughts.  I'm sure there'll be
>>> more as I read the implementation.
>>>
>>
>> Thanks for starting to look at this so quickly.
> NP.  Your timing to come back to this is good.
> 
> Jeff
>