Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Jan Beulich <JBeulich@xxxxxxxx>]

On 29.07.2019 17:55, Andrew Cooper wrote:
> On 29/07/2019 14:51, Jan Beulich wrote:
>> On 26.07.2019 22:32, Andrew Cooper wrote:
>>> --- a/xen/include/asm-x86/processor.h
>>> +++ b/xen/include/asm-x86/processor.h
>>> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, 
>>> unsigned long ecx)
>>>    #define IOBMP_BYTES             8192
>>>    #define IOBMP_INVALID_OFFSET    0x8000
>>>    
>>> -struct __packed __cacheline_aligned tss_struct {
>>> +struct __packed tss_struct {
>>>        uint32_t :32;
>>>        uint64_t rsp0, rsp1, rsp2;
>>>        uint64_t :64;
>>> @@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
>>>        /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>>>        uint8_t __cacheline_filler[24];
>>>    };
>>> +DECLARE_PER_CPU(struct tss_struct, init_tss);
>> Taking patch 1 this expands to
>>
>>       __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
>>                        __aligned(PAGE_SIZE), struct tss_struct, _init_tss);
>>
>> and then
>>
>>       __section(".bss.percpu.page_aligned") __aligned(PAGE_SIZE)
>>       __typeof__(struct tss_struct) per_cpu__init_tss;
>>
>> which is not what you want: You have an object of size
>> sizeof(struct tss_struct) which is PAGE_SIZE aligned. Afaict you
>> therefore still leak everything that follows in the same page.
> 
> What data might this be?

Whatever sits early enough in .bss.percpu. There's no page size
alignment enforced between the two sections, ...

> Every object put into this section is suitably aligned, so nothing will
> sit in the slack between the TSS and the end of the page.

... so for the object being last in .bss.percpu.page_aligned it
is its size that matters, not its alignment.

>> Perhaps the solution is a two-layer approach:
>>
>> struct __aligned(PAGE_SIZE) xen_tss {
>>       struct __packed tss_struct {
>>           ...
>>       };
>> };
>>
>> where the inner structure describes the hardware interface and the
>> containing one our own requirement(s). But personally I also
>> wouldn't mind putting the __aligned(PAGE_SIZE) right on struct
>> tss_struct, where __cacheline_aligned has been sitting.
> 
> The only way that would make things more robust is if xen_tss was a
> union with char[4096] to extend its size.
> 
> However, I think this is overkill, given the internals of
> DEFINE_PER_CPU_PAGE_ALIGNED()
> 
>> Of course either approach goes against the idea of avoiding usage
>> mistakes (as pointed out by others in the v1 discussion, iirc):
>> There better wouldn't be a need to get the two "page aligned"
>> attributes in sync, i.e. the instantiation of the structure
>> would better enforce the requested alignment. I've not thought
>> through whether there's trickery to actually make this work, but
>> I'd hope we could at the very least detect things not being in
>> sync at compile time.
> 
> There is a reason why I put in a linker assertion for the TSS being
> non-aligned.

Hmm, I indeed didn't have that one on my radar when writing the reply.
However, a compile time diagnostic was what I would prefer: Having to
add linker assertions for each and every object we put in this new
section wouldn't really be nice, even if right now we certainly hope
for this to be the only such object.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel