Re: [PATCH v4.1 13/14] x86: Clamp bits in eflags more aggressively

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/03/2026 8:15 am, Jan Beulich wrote:
> On 11.03.2026 18:58, Andrew Cooper wrote:
>> In FRED mode, ERET is stricter than IRET about flags.  Notably this means:
>>
>>  * The vm86 bit (bit 17) and IOPL (bits 12,13) must be clear.
>>  * The sticky-1 reserved bit (bit 2) must be set, so dom0_construct() needs 
>> to
>>    set X86_EFLAGS_MBS in order for a PV dom0 to start.
>>  * All other reserved bits must be clear.
>>
>> Xen has been overly lax with reserved bit handling.  Adjust
>> arch_set_info_guest*() and hypercall_iret() which consume flags to clamp the
>> reserved bits for all guest types.
>>
>> This is a minor ABI change, but by the same argument as commit
>> 9f892f84c279 ("x86/domctl: Stop using XLAT_cpu_user_regs()"); the reserved
>> bits would get clamped like this naturally by hardware when the vCPU is run.
>>
>> The handling of vm86 is also different.  Guests under 32bit Xen really could
>> use vm86 mode, but Long Mode disallows vm86 mode and IRET simply ignores the
>> bit.  Xen's behaviour for a PV32 guest trying to use vm86 mode under a 64bit
>> Xen is to arrange to deliver #GP at the target of the IRET, rather than to
>> fail the IRET itself.
>>
>> However there's no filter filtering in arch_set_info_guest() itself, and it
> Nit: Excess "filter"?

Yes.  I noticed that immediately after sending.

>
>> can't arrange to queue a #GP at the target, so do the next best thing and 
>> fail
>> the hypercall.  This is not expected to create an issue for PV guests, as the
>> result of such an arch_set_info_guest() previously would be to run supposedly
>> Real Mode code as Protected Mode code.
>>
>> This allows PV guests to start when Xen is using FRED mode.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

>
> Nevertheless, a largely unrelated remark and two suggestions:
>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1193,6 +1193,14 @@ int arch_set_info_guest(
>>  
>>              if ( !__addr_ok(c.nat->ldt_base) )
>>                  return -EINVAL;
> Seeing this still in context: I had some trouble locating the position where
> you're making the change, as in my local tree this is long gone. Is there
> any chance we could make progress on "x86/PV: consolidate LDT checks" [1]?

I'll have another look, but this patch is going to need to go in first
as it needs backporting to 4.21.

>
>> +
>> +            /*
>> +             * IRET in Long Mode discards EFLAGS.VM, but in FRED mode ERET
>> +             * cares that it is zero.
>> +             *
>> +             * Guests can't see FRED, so emulate IRET behaviour.
>> +             */
>> +            c.nat->user_regs.rflags &= ~X86_EFLAGS_VM;
>>          }
>>  #ifdef CONFIG_COMPAT
>>          else
>> @@ -1205,6 +1213,18 @@ int arch_set_info_guest(
>>  
>>              for ( i = 0; i < ARRAY_SIZE(c.cmp->trap_ctxt); i++ )
>>                  fixup_guest_code_selector(d, c.cmp->trap_ctxt[i].cs);
>> +
>> +            /*
>> +             * Under 32bit Xen, PV guests could really use vm86 mode.  Under
>> +             * 64bit Xen, vm86 mode can't be entered even by PV32 guests.
>> +             *
>> +             * For backwards compatibility, compat HYPERCALL_iret will 
>> arrange
>> +             * to deliver #GP at the target of the IRET rather than to fail
>> +             * the IRET itself, but we can't arrange for the same behaviour
>> +             * here.  Reject the hypercall as the next best option.
>> +             */
>> +            if ( c.cmp->user_regs.eflags & X86_EFLAGS_VM )
>> +                return -EINVAL;
> Technically we could support VM86 mode, by fully emulating things. Hence I
> think -EOPNOTSUPP would be more appropriate.

Sorry, but I think you're rather too late on that suggestion.  Anyone
wanting vm86 mode can use a VM.

>
>>          }
>>  #endif
> Having all of the EFLAGS handling together would be nice. IOPL and IF handling
> sit further down. Could I talk you into moving these additions down there?

No, but not for ...

>  Yes,
> there are downsides to that: It looks to need another "compat" conditional, 
> and
> it would further the mix of state updates and error checks. Yet I still think
> having all of the EFLAGS stuff together is a benefit.

... these reasons.  The later position is after the point at which it's
buggy to fail the hypercall, because we've already reset the FPU amongst
other things.

This is a dire function in need of a lot of work.  I'm just leaving it
no more broken than it was before.

~Andrew