Re: [PATCH] x86/traps: Drop incorrect BUILD_BUG_ON() and comment in load_system_tables()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.03.2025 13:29, Andrew Cooper wrote:
> On 17/03/2025 9:03 am, Jan Beulich wrote:
>> On 14.03.2025 19:33, Andrew Cooper wrote:
>>> It is only the hardware task switching mechanism which cares about a TSS 
>>> being
>>> at least 0x67 bytes long.
>> I/O bitmap accesses are where this particular limit comes into play. For
>> 32-bit task switching a slightly shorter one would still do, I think?
> 
> Even by x86 standards its a terrible hack.  32-bit task switching
> mandates 0x67, even though the IO bitmap is not accessed for the
> outgoing or incoming task.
> 
> For IO accesses in general, a limit shorter than the IO bitmap pointer
> means no IO bitmap, and IO accesses in Ring3 take #GP.
> 
>>
>>>  Furthermore, since this check was added, the limit is now 0x6b if CET-SS is
>>> active.
>> Which isn't reflected at all in struct tss64: Aiui that's an addition to the
>> 32-bit TSS only.
> 
> 0x67 isn't relevant to tss64 either.  It's strictly for hardware task
> switching, which is strictly for 32bit.
> 
> 
>>> --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -900,8 +900,6 @@ void load_system_tables(void)
>>>             wrmsrl(MSR_INTERRUPT_SSP_TABLE, (unsigned long)ist_ssp);
>>>     }
>>>  
>>> -   BUILD_BUG_ON(sizeof(*tss) <= 0x67); /* Mandated by the architecture. */
>>> -
>>>     _set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>>                      sizeof(*tss) - 1, SYS_DESC_tss_avail);
>> All of the above said, the removal worries me primarily with the sizeof()
>> still in use here.
> 
> Xen uses IST4 but not IST5.
> 
> Xen could set the limit to 67 (== 0x43) and everything would continue to
> be fine.  In fact, this is quite possibly a better option than poisoning
> IST[5..7].
> 
> I'm deleting the BUILD_BUG_ON() because everything about it, even the
> comment, is incorrect for Xen.

Okay, yet then there's still that sizeof() use, and iirc that's the sole reason
why the BUILD_BUG_ON() was originally put there. Now we can of course promise
to ourselves that we won't ever edit struct tss64 in an undue way. On that
basis dropping one but not the other is perhaps okay-ish. Irrespective I think
that the better route would be to replace the other sizeof() as well. Perhaps,
as you say, to even lower the limit. (Provided that doesn't run into bugs on,
say, qemu. Given how well all of this is documented, finding bugs in emulation
code wouldn't be entirely unexpected.)

Jan