On 05.01.2026 17:39, Andrew Cooper wrote:
> On 05/01/2026 4:13 pm, Nicola Vetrini wrote:
>> On 2026-01-05 16:52, Jan Beulich wrote:
>>> On 30.12.2025 14:54, Andrew Cooper wrote:
>>>> Use asm goto rather than hiding a memset() in the fixup section.
>>>> With the
>>>> compiler now able to see the write into fpu_ctxt (as opposed to the asm
>>>> constraint erroneously stating it as input-only), it validly objects
>>>> to the
>>>> pointer being const.
>>>>
>>>> While FXRSTOR oughtn't to fault on an all-zeros input, avoid a risk
>>>> of an
>>>> infinite loop entirely by using a fixup scheme similar to xrstor(), and
>>>> crashing the domain if we run out options.
>>>
>>> Question being - does ...
>>>
>>>> --- a/xen/arch/x86/i387.c
>>>> +++ b/xen/arch/x86/i387.c
>>>> @@ -38,7 +38,8 @@ static inline void fpu_xrstor(struct vcpu *v,
>>>> uint64_t mask)
>>>> /* Restore x87 FPU, MMX, SSE and SSE2 state */
>>>> static inline void fpu_fxrstor(struct vcpu *v)
>>>> {
>>>> - const fpusse_t *fpu_ctxt = &v->arch.xsave_area->fpu_sse;
>>>> + fpusse_t *fpu_ctxt = &v->arch.xsave_area->fpu_sse;
>>>> + unsigned int faults = 0;
>>>>
>>>> /*
>>>> * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
>>>> @@ -59,49 +60,41 @@ static inline void fpu_fxrstor(struct vcpu *v)
>>>> * possibility, which may occur if the block was passed to us
>>>> by control
>>>> * tools or through VCPUOP_initialise, by silently clearing the
>>>> block.
>>>> */
>>>> + retry:
>>>> switch ( __builtin_expect(fpu_ctxt->x[FPU_WORD_SIZE_OFFSET], 8) )
>>>> {
>>>> default:
>>>> - asm_inline volatile (
>>>> + asm_inline volatile goto (
>>>> "1: fxrstorq %0\n"
>>>> - ".section .fixup,\"ax\" \n"
>>>> - "2: push %%"__OP"ax \n"
>>>> - " push %%"__OP"cx \n"
>>>> - " push %%"__OP"di \n"
>>>> - " lea %0,%%"__OP"di \n"
>>>> - " mov %1,%%ecx \n"
>>>> - " xor %%eax,%%eax \n"
>>>> - " rep ; stosl \n"
>>>> - " pop %%"__OP"di \n"
>>>> - " pop %%"__OP"cx \n"
>>>> - " pop %%"__OP"ax \n"
>>>> - " jmp 1b \n"
>>>> - ".previous \n"
>>>> - _ASM_EXTABLE(1b, 2b)
>>>> - :
>>>> - : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4) );
>>>> + _ASM_EXTABLE(1b, %l[fault])
>>>> + :: "m" (*fpu_ctxt)
>>>> + :: fault );
>>>> break;
>>>> +
>>>> case 4: case 2:
>>>> - asm_inline volatile (
>>>> - "1: fxrstor %0 \n"
>>>> - ".section .fixup,\"ax\"\n"
>>>> - "2: push %%"__OP"ax \n"
>>>> - " push %%"__OP"cx \n"
>>>> - " push %%"__OP"di \n"
>>>> - " lea %0,%%"__OP"di \n"
>>>> - " mov %1,%%ecx \n"
>>>> - " xor %%eax,%%eax \n"
>>>> - " rep ; stosl \n"
>>>> - " pop %%"__OP"di \n"
>>>> - " pop %%"__OP"cx \n"
>>>> - " pop %%"__OP"ax \n"
>>>> - " jmp 1b \n"
>>>> - ".previous \n"
>>>> - _ASM_EXTABLE(1b, 2b)
>>>> - :
>>>> - : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4) );
>>>> + asm_inline volatile goto (
>>>> + "1: fxrstor %0\n"
>>>> + _ASM_EXTABLE(1b, %l[fault])
>>>> + :: "m" (*fpu_ctxt)
>>>> + :: fault );
>>>> break;
>>>> }
>>>> +
>>>> + return;
>>>> +
>>>> + fault:
>>>> + faults++;
>>>> +
>>>> + switch ( faults )
>>>> + {
>>>> + case 1: /* Stage 1: Reset all state. */
>>>> + memset(fpu_ctxt, 0, sizeof(*fpu_ctxt));
>>>> + goto retry;
>>>> +
>>>> + default: /* Stage 2: Nothing else to do. */
>>>> + domain_crash(v->domain, "Uncorrectable FXRSTOR fault\n");
>>>> + return;
>>>
>>> ... this then count as unreachable and/or dead code in Misra's terms?
>>> Nicola?
>>> Sure, Eclair wouldn't be able to spot it, but that's no excuse imo.
>>
>> Right now, probably not, but even if it did, an ASSERT_UNREACHABLE can
>> be added in the default branch to deal with that.
>
> It's fully reachable.
>
> FXRSTOR can fault multiple times, and can fault for reasons unrelated to
> the contents of the buffer. Fault recovery isn't even limited to only
> #GP[0] only, and FXRSTOR can suffer #AC from a misaligned pointer.
None of these faults are what we mean to recover from here. Faults
unrelated to buffer contents would pretty likely occur on the memset()
as well.
As to #AC - in ring 3, but not in ring 0 (where Xen runs)?
> If Xen is operating properly, it oughtn't to fault more than once, but
> right now the logic will livelock rather than terminate.
s/will/would/ as that's only hypothetical (assuming no other bugs).
> Further fixes being discussed (better auditing of toolstack-provided
> buffers) should cause it never to fault for buffer-contents reasons, at
> which point I'll be removing the retry aspect and escalating to
> domain_crash() unconditionally.
Still in the meantime I think Nicola's suggestion should be taken
and ASSERT_UNREACHABLE() be added. Then
Acked-by: Jan Beulich <[email protected]>
Jan
