Re: [Xen-devel] [PATCH v2] SVM: limit GIF=0 region

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/08/18 07:26, Jan Beulich wrote:
>>>> On 29.08.18 at 01:06, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 15/08/18 07:09, Jan Beulich wrote:
>>> @@ -96,13 +101,12 @@ __UNLIKELY_END(nsvm_hap)
>>>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, 
>>> Clob: acd */
>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>>  
>>> -        STGI
>>> -GLOBAL(svm_stgi_label)
>>> +        sti
>> Nack.  As indicated in v1, moving this breaks SPEC_CTRL_ENTRY_FROM_HVM
>> (Even if there is an unexpected bug on the VT-x side of things which
>> needs fixing differently).
> Well, this increases the pressure to fix the issue. I very much
> expect that fix to also take care of the code here. It was
> therefore intentional that I did only the technically necessary
> adjustments in v2.
>
> I would, btw, likely have tried to find time to look into fixing
> that issue, but so far I was under the impression that you are
> planning to, and since you've written the code I thought you'd
> likely also be in a better position to do so.

Its somewhere in the massive pile of still-security work that needs
doing.  It is not the most urgent of the still-security work to do,
partly because this protection is currently in place.  Top of the list,
and most important, is still the toolstack CPUID/MSR interactions so we
can feasibly tell a guest what it needs to know WRT L1TF.

>
>> Furthermore, to fix LBR handling, the first thing I'd have to do is
>> revert this, so please leave it as it is.
> Mind being a little more specific as to the whys here?

When vmcb->virt_ext.fields.lbr_enable is clear and DEBUGCTL.LBR is set,
the hypervisor must atomically switch the 5 LBR MSRs inside the critical
region.

Usage of DEBUGCTL.LBR is broken on pre-lbrv hardware (gen 1/2 svm?), and
when suitably (mis)configured by nested virt.

Other bugs include Xen's LBR setting leaking into guests.

> From a purely formal pov, Boris'es R-b allows me to put the change in
> as is. I'm not overly happy to do the requested change, but I
> certainly will, provided - once again - I understand the reason.
>
> Furthermore, please don't forget that the more we delay
> especially #MC, the more likely it becomes that a system will
> crash in a far less obvious way than by logging an #MC.

#MC's only get raised after the bank registers have been updated, so
even if a shutdown occurs, the data will be captured by the firmware
after reset.

As for non-deferrable #MC's, a couple of extra stack/msr/vcpu references
in the critical region doesn't alter the chances.  We were never going
to survive, or won't trigger a new one.

~Andrew

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