Re: [PATCH 1/9] x86emul: support LKGS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2023 23:54, Andrew Cooper wrote:
> On 04/04/2023 3:49 pm, Jan Beulich wrote:
>> Provide support for this insn, which is a prereq to FRED. CPUID-wise
>> introduce both its and FRED's bit at this occasion, thus allowing to
>> also express the dependency right away.
>>
>> While adding a testcase, also add a SWAPGS one. In order to not affect
>> the behavior of pre-existing tests, install write_{segment,msr} hooks
>> only transiently.
> 
> IMO, the emulator is already complicated enough without us having
> fallback logic to cope with callers that don't set up all the hooks.
> 
> Nor do I think making these hooks transient in the test harness is a
> clever idea.

Are you suggesting we start to _require_ all hooks to be set? That'll
mean many useless stubs in particular in PV emulation. Furthermore
absent hooks sometimes cause default behavior to apply rather than
outright failure, so altering what hooks are present can affect
overall behavior. Hence the transient establishing of the two hooks
here.

>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Instead of ->read_segment() we could of course also use ->read_msr() to
>> fetch the original GS base. I don't think I can see a clear advantage of
>> either approach; the way it's done it matches how we handle SWAPGS.
> 
> read_segment() is a much shorter logic chain under the hood, so will be
> marginally faster, but it will be a couple of unnecessary VMREADs (on
> Intel at least).

And this is precisely why I think it's not entirely clear. Anyway,
the remark is here just in case you (or Roger) thought doing it the
other way might be better.

> We could expose the get/set reg paths for cases where we know we're not
> going to need sanity checks, but I'm not sure it's worth it in this case.

Probably not.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2886,8 +2886,31 @@ x86_emulate(
>>                  break;
>>              }
>>              break;
>> -        default:
>> -            generate_exception_if(true, EXC_UD);
>> +        case 6: /* lkgs */
>> +            generate_exception_if((modrm_reg & 1) || vex.pfx != vex_f2, 
>> EXC_UD);
>> +            generate_exception_if(!mode_64bit() || !mode_ring0(), EXC_UD);
> 
> Can we switch to X86_* please.  Alternatively, I've got such a patch
> which I've just rebased over all your emulator changes anyway, if we're
> happy to fix this in one fell swoop.

Of course, and I've applied this transformation to all the emulator
patches I have pending (i.e. no need to re-request this elsewhere).

> (Sadly, you did move some TRAP_* names into util-xen.c which I fixed up
> in my other tree-wide exception constant patch.)

Hmm, yes, I changed EXC_* -> X86_EXC_* but failed to pay attention to
TRAP_* (because there no build issue arose).

>> +            vcpu_must_have(lkgs);
>> +            fail_if(!ops->read_segment || !ops->read_msr ||
>> +                    !ops->write_segment || !ops->write_msr);
>> +            if ( (rc = ops->read_msr(MSR_SHADOW_GS_BASE, &msr_val,
>> +                                     ctxt)) != X86EMUL_OKAY ||
>> +                 (rc = ops->read_segment(x86_seg_gs, &sreg,
>> +                                         ctxt)) != X86EMUL_OKAY )
>> +                goto done;
>> +            dst.orig_val = sreg.base;
>> +            if ( (rc = protmode_load_seg(x86_seg_gs, src.val, false, &sreg,
>> +                                         ctxt, ops)) != X86EMUL_OKAY ||
>> +                 (rc = ops->write_msr(MSR_SHADOW_GS_BASE, sreg.base,
>> +                                      ctxt)) != X86EMUL_OKAY )
>> +                goto done;
>> +            sreg.base = dst.orig_val;
> 
> Honestly, I think a comment is needed here, because I'm struggling to
> work out if this is correct or not.
> 
> There is a 64->32 bit truncation of base with LGKS, just as there is
> with MOV GS.
> 
> Which I think does happen as a side effect of protmode_load_seg() only
> filling in the lower half of sreg.base,

I thought that's obvious, as protmode_load_seg() can't possibly have
any other behavior. But ...

> but I think it would be nicer to
> have:
> 
> +            dst.orig_val = sreg.base; /* Preserve full GS Base */
> +            if ( (rc = protmode_load_seg(x86_seg_gs, src.val, false, &sreg,
> +                                         ctxt, ops)) != X86EMUL_OKAY ||
> +                 /* Write truncated base into GS_SHADOW */
> +                 (rc = ops->write_msr(MSR_SHADOW_GS_BASE, sreg.base,
> +                                      ctxt)) != X86EMUL_OKAY )
> +                goto done;
> +            sreg.base = dst.orig_val; /* Reinstate full GS Base */
> 
> Or so, because it's weird not to see a (uint32_t) somewhere in this logic.

... sure, I've added comments. I don't think "truncated" is correct,
as there's no truncation - there's no more than 32 bits we can read
out of the GDT/LDT. I've used "32-bit" instead.

>> +            if ( (rc = ops->write_segment(x86_seg_gs, &sreg,
>> +                                          ctxt)) != X86EMUL_OKAY )
>> +            {
>> +                /* Best effort unwind (i.e. no error checking). */
>> +                ops->write_msr(MSR_SHADOW_GS_BASE, msr_val, ctxt);
> 
> write_segment() can't fail.  (The sanity checks are actually deferred
> until after emulation is complete, and I'm not sure if that's behaviour
> we want...)

Irrespective we shouldn't start omitting error checks. If we truly
mean write_segment() to always be successful, we should make it
return "void". Yet I wouldn't view such as a good move.

> However, more importantly, if we actually take this error path (for some
> future reason) then we've created a security vulnerability in the guest.

You mean in case additionally the write_msr() also fails? In any
event, ...

> It will be strictly better to crash the domain in this case, than to try
> and let it continue in this state.

... we don't return OKAY in this case, so in most cases the guest
will already be crashed, won't it? Otherwise it's not really clear
to me what action you propose to take to "crash the domain": Right
here we better wouldn't call domain_crash() (or else we'd need yet
another #ifdef __XEN__ around it).

Furthermore - what does what you say here mean for the (existing)
similar code path in SWAPGS handling?

>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -295,6 +295,9 @@ def crunch_numbers(state):
>>  
>>          # In principle the TSXLDTRK insns could also be considered 
>> independent.
>>          RTM: [TSXLDTRK],
>> +
>> +        # FRED builds on the LKGS instruction.
>> +        LKGS: [FRED],
> 
> Hmm...  This is the first case (I think) we've got where a dependency
> that goes back numerically in terms of feature number.
> 
> Obviously we need to support it, but I'm not sure if the deep_deps loop
> will cope in its current form.

As you know my Python isn't overly good, but looking at the loop
makes me think it deals with this fine; I can't see an ordering
dependency. Looking at the generated INIT_DEEP_DEPS appears to
confirm this - there is an entry for LKGS, and while I haven't
counted elements, the set bit there looks to be in a plausible
position. The only thing I'm not entirely certain about is whether
a further (hypothetical) transitive dependency would also be dealt
with correctly.

If there was an issue here, I'd really like to leave addressing it
to you, as your Python is surely much better than mine.

Jan