At the time of XSA-170, the x86 instruction emulator was genuinely broken. It
would load arbitrary values into %rip and putting a check here probably was
the best stopgap security fix. It should have been reverted following c/s
81d3a0b26c1 "x86emul: limit-check branch targets" which corrected the emulator
behaviour.
However, everyone involved in XSA-170, myself included, failed to read the SDM
correctly. On the subject of %rip consistency checks, the SDM stated:
If the processor supports N < 64 linear-address bits, bits 63:N must be
identical
A non-canonical %rip (and SSP more recently) is an explicitly legal state in
x86, and the VMEntry consistency checks are intentionally off-by-one from a
regular canonical check.
The consequence of this bug is that Xen will currently take a legal x86 state
which would successfully VMEnter, and corrupt it into having non-architectural
behaviour.
Furthermore, in the time this bugfix has been pending in public, I
successfully persuaded Intel to clarify the SDM, adding the following
clarification:
The guest RIP value is not required to be canonical; the value of bit N-1
may differ from that of bit N.
Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
Rewrite the commit message, but no change to the fix.
Fun fact... LAM almost required this to be reverted in order to support, but
the penultimate draft of the spec backed down and made LAM only apply to data
accesses, not instruction fetches.
---
xen/arch/x86/hvm/vmx/vmx.c | 34 +---------------------------------
1 file changed, 1 insertion(+), 33 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bfc9693f7e43..79ff59b11c6b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4035,7 +4035,7 @@ static void undo_nmis_unblocked_by_iret(void)
void vmx_vmexit_handler(struct cpu_user_regs *regs)
{
unsigned long exit_qualification, exit_reason, idtv_info, intr_info = 0;
- unsigned int vector = 0, mode;
+ unsigned int vector = 0;
struct vcpu *v = current;
struct domain *currd = v->domain;
@@ -4730,38 +4730,6 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
out:
if ( nestedhvm_vcpu_in_guestmode(v) )
nvmx_idtv_handling();
-
- /*
- * VM entry will fail (causing the guest to get crashed) if rIP (and
- * rFLAGS, but we don't have an issue there) doesn't meet certain
- * criteria. As we must not allow less than fully privileged mode to have
- * such an effect on the domain, we correct rIP in that case (accepting
- * this not being architecturally correct behavior, as the injected #GP
- * fault will then not see the correct [invalid] return address).
- * And since we know the guest will crash, we crash it right away if it
- * already is in most privileged mode.
- */
- mode = vmx_guest_x86_mode(v);
- if ( mode == 8 ? !is_canonical_address(regs->rip)
- : regs->rip != regs->eip )
- {
- gprintk(XENLOG_WARNING, "Bad rIP %lx for mode %u\n", regs->rip, mode);
-
- if ( vmx_get_cpl() )
- {
- __vmread(VM_ENTRY_INTR_INFO, &intr_info);
- if ( !(intr_info & INTR_INFO_VALID_MASK) )
- hvm_inject_hw_exception(X86_EXC_GP, 0);
- /* Need to fix rIP nevertheless. */
- if ( mode == 8 )
- regs->rip = (long)(regs->rip << (64 - VADDR_BITS)) >>
- (64 - VADDR_BITS);
- else
- regs->rip = regs->eip;
- }
- else
- domain_crash(v->domain);
- }
}
static void lbr_tsx_fixup(void)
--
2.30.2
On Wed, Apr 05, 2023 at 10:52:45PM +0100, Andrew Cooper wrote:
> At the time of XSA-170, the x86 instruction emulator was genuinely broken. It
> would load arbitrary values into %rip and putting a check here probably was
> the best stopgap security fix. It should have been reverted following c/s
> 81d3a0b26c1 "x86emul: limit-check branch targets" which corrected the emulator
> behaviour.
>
> However, everyone involved in XSA-170, myself included, failed to read the SDM
> correctly. On the subject of %rip consistency checks, the SDM stated:
>
> If the processor supports N < 64 linear-address bits, bits 63:N must be
> identical
>
> A non-canonical %rip (and SSP more recently) is an explicitly legal state in
> x86, and the VMEntry consistency checks are intentionally off-by-one from a
> regular canonical check.
>
> The consequence of this bug is that Xen will currently take a legal x86 state
> which would successfully VMEnter, and corrupt it into having non-architectural
> behaviour.
>
> Furthermore, in the time this bugfix has been pending in public, I
> successfully persuaded Intel to clarify the SDM, adding the following
> clarification:
>
> The guest RIP value is not required to be canonical; the value of bit N-1
> may differ from that of bit N.
>
> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
I think the fixes tag should likely be "x86emul: limit-check branch
targets", since it's that commit that missed the revert done here?
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
On 23/08/2023 12:15 pm, Roger Pau Monné wrote:
> On Wed, Apr 05, 2023 at 10:52:45PM +0100, Andrew Cooper wrote:
>> At the time of XSA-170, the x86 instruction emulator was genuinely broken. It
>> would load arbitrary values into %rip and putting a check here probably was
>> the best stopgap security fix. It should have been reverted following c/s
>> 81d3a0b26c1 "x86emul: limit-check branch targets" which corrected the emulator
>> behaviour.
>>
>> However, everyone involved in XSA-170, myself included, failed to read the SDM
>> correctly. On the subject of %rip consistency checks, the SDM stated:
>>
>> If the processor supports N < 64 linear-address bits, bits 63:N must be
>> identical
>>
>> A non-canonical %rip (and SSP more recently) is an explicitly legal state in
>> x86, and the VMEntry consistency checks are intentionally off-by-one from a
>> regular canonical check.
>>
>> The consequence of this bug is that Xen will currently take a legal x86 state
>> which would successfully VMEnter, and corrupt it into having non-architectural
>> behaviour.
>>
>> Furthermore, in the time this bugfix has been pending in public, I
>> successfully persuaded Intel to clarify the SDM, adding the following
>> clarification:
>>
>> The guest RIP value is not required to be canonical; the value of bit N-1
>> may differ from that of bit N.
>>
>> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
> I think the fixes tag should likely be "x86emul: limit-check branch
> targets", since it's that commit that missed the revert done here?
Well, not really. ffbbfda377 really does have a bug, irrespective of
the changes in the emulator.
The presence of 81d3a0b26c1 is why this bugfix is a full revert of
ffbbfda377, and not just an off-by-1 adjustment.
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks.
~Andrew
On Wed, Aug 23, 2023 at 12:56:48PM +0100, Andrew Cooper wrote:
> On 23/08/2023 12:15 pm, Roger Pau Monné wrote:
> > On Wed, Apr 05, 2023 at 10:52:45PM +0100, Andrew Cooper wrote:
> >> At the time of XSA-170, the x86 instruction emulator was genuinely broken. It
> >> would load arbitrary values into %rip and putting a check here probably was
> >> the best stopgap security fix. It should have been reverted following c/s
> >> 81d3a0b26c1 "x86emul: limit-check branch targets" which corrected the emulator
> >> behaviour.
> >>
> >> However, everyone involved in XSA-170, myself included, failed to read the SDM
> >> correctly. On the subject of %rip consistency checks, the SDM stated:
> >>
> >> If the processor supports N < 64 linear-address bits, bits 63:N must be
> >> identical
> >>
> >> A non-canonical %rip (and SSP more recently) is an explicitly legal state in
> >> x86, and the VMEntry consistency checks are intentionally off-by-one from a
> >> regular canonical check.
> >>
> >> The consequence of this bug is that Xen will currently take a legal x86 state
> >> which would successfully VMEnter, and corrupt it into having non-architectural
> >> behaviour.
> >>
> >> Furthermore, in the time this bugfix has been pending in public, I
> >> successfully persuaded Intel to clarify the SDM, adding the following
> >> clarification:
> >>
> >> The guest RIP value is not required to be canonical; the value of bit N-1
> >> may differ from that of bit N.
> >>
> >> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
> > I think the fixes tag should likely be "x86emul: limit-check branch
> > targets", since it's that commit that missed the revert done here?
>
> Well, not really. ffbbfda377 really does have a bug, irrespective of
> the changes in the emulator.
>
> The presence of 81d3a0b26c1 is why this bugfix is a full revert of
> ffbbfda377, and not just an off-by-1 adjustment.
Right, but taking this patch without also having 81d3a0b26c1 will lead
to a vulnerable system, hence why I think the dependency would better
be on 81d3a0b26c1.
Anyway, I don't think it's worth arguing over, so if you want to leave
it as-is I won't object.
Thanks, Roger.
On 23/08/2023 2:31 pm, Roger Pau Monné wrote:
> On Wed, Aug 23, 2023 at 12:56:48PM +0100, Andrew Cooper wrote:
>> On 23/08/2023 12:15 pm, Roger Pau Monné wrote:
>>> On Wed, Apr 05, 2023 at 10:52:45PM +0100, Andrew Cooper wrote:
>>>> At the time of XSA-170, the x86 instruction emulator was genuinely broken. It
>>>> would load arbitrary values into %rip and putting a check here probably was
>>>> the best stopgap security fix. It should have been reverted following c/s
>>>> 81d3a0b26c1 "x86emul: limit-check branch targets" which corrected the emulator
>>>> behaviour.
>>>>
>>>> However, everyone involved in XSA-170, myself included, failed to read the SDM
>>>> correctly. On the subject of %rip consistency checks, the SDM stated:
>>>>
>>>> If the processor supports N < 64 linear-address bits, bits 63:N must be
>>>> identical
>>>>
>>>> A non-canonical %rip (and SSP more recently) is an explicitly legal state in
>>>> x86, and the VMEntry consistency checks are intentionally off-by-one from a
>>>> regular canonical check.
>>>>
>>>> The consequence of this bug is that Xen will currently take a legal x86 state
>>>> which would successfully VMEnter, and corrupt it into having non-architectural
>>>> behaviour.
>>>>
>>>> Furthermore, in the time this bugfix has been pending in public, I
>>>> successfully persuaded Intel to clarify the SDM, adding the following
>>>> clarification:
>>>>
>>>> The guest RIP value is not required to be canonical; the value of bit N-1
>>>> may differ from that of bit N.
>>>>
>>>> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
>>> I think the fixes tag should likely be "x86emul: limit-check branch
>>> targets", since it's that commit that missed the revert done here?
>> Well, not really. ffbbfda377 really does have a bug, irrespective of
>> the changes in the emulator.
>>
>> The presence of 81d3a0b26c1 is why this bugfix is a full revert of
>> ffbbfda377, and not just an off-by-1 adjustment.
> Right, but taking this patch without also having 81d3a0b26c1 will lead
> to a vulnerable system, hence why I think the dependency would better
> be on 81d3a0b26c1.
>
> Anyway, I don't think it's worth arguing over, so if you want to leave
> it as-is I won't object.
We don't really have a depends-on tag, or only-safe-with or whatever,
and a change like that is stretching the definition of Fixes IMO.
81d3a0b26c1 is more than 7 years old now, so there's not going to be a
practical problem backporting. For everything else, I expect people to
read the commit message and apply common sense.
~Andrew
On 05.04.2023 23:52, Andrew Cooper wrote:
> At the time of XSA-170, the x86 instruction emulator was genuinely broken. It
> would load arbitrary values into %rip and putting a check here probably was
> the best stopgap security fix. It should have been reverted following c/s
> 81d3a0b26c1 "x86emul: limit-check branch targets" which corrected the emulator
> behaviour.
>
> However, everyone involved in XSA-170, myself included, failed to read the SDM
> correctly. On the subject of %rip consistency checks, the SDM stated:
>
> If the processor supports N < 64 linear-address bits, bits 63:N must be
> identical
>
> A non-canonical %rip (and SSP more recently) is an explicitly legal state in
> x86, and the VMEntry consistency checks are intentionally off-by-one from a
> regular canonical check.
>
> The consequence of this bug is that Xen will currently take a legal x86 state
> which would successfully VMEnter, and corrupt it into having non-architectural
> behaviour.
>
> Furthermore, in the time this bugfix has been pending in public, I
> successfully persuaded Intel to clarify the SDM, adding the following
> clarification:
>
> The guest RIP value is not required to be canonical; the value of bit N-1
> may differ from that of bit N.
>
> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
I am kind of okay with such a full revert, but I'd consider it quite helpful
if the description made clear why the alternative of instead doing the spec
mandated check isn't necessary / useful. The emulator having gained respective
checking is only part of the reason for this, aiui. Plus bugs may be
introduced into the emulator again, where the checking here could be a guard
against needing to issue an XSA in such a case.
Jan
© 2016 - 2024 Red Hat, Inc.