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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/08/2019 13:32, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> TBD: Especially with how the previous patch now works I'm unconvinced of
>      the utility of the linker script alignment check. It in particular
>      doesn't check the property we're after in this patch, i.e. the fact
>      that there's nothing else in the same page.

It should now probably be a BUILD_BUG_ON() checking sizeof(tss_page)
being exactly a page, given that there is also a compile time alignment
assertion.

> NB: Sadly get_per_cpu_var() can't also be used on the "left" side of a
>     #define.

I don't understand what you are trying to imply with this.  That said, ...

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsign
>  #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,11 @@ struct __packed __cacheline_aligned tss_
>      /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>      uint8_t __cacheline_filler[24];
>  };
> +struct tss_page {
> +    struct tss_struct __aligned(PAGE_SIZE) tss;
> +};
> +DECLARE_PER_CPU(struct tss_page, init_tss_page);
> +#define per_cpu__init_tss get_per_cpu_var(init_tss_page.tss)

... my first attempt started by renaming init_tss because the init part
is bogus.  I believe we inherited this nomenclature from Linux, but that
doesn't make it right.

I referred the renaming patch specifically to aid in backportability,
but given these changes to the type, leaving it alone isn't an option.

I'm not happy with introducing this diversion, because it means that all
users of per_cpu(init_tss) now have the wrong types in their hand from
the point of view of reading the code.

I'm still uncertain which is the least bad option between backporting
this version, and backporting the version which adjusts all users -
there aren't too many, and its definitely not the most awkward backport
we've ever had to do.

I could be persuaded into keeping this version for legacy trees, so long
as it doesn't propagate into master.  i.e. this patch drops the init_
prefix, and I'll rebase my renaming as a 3/2 again which gets committed
at around about the same time.

That way we retain a non-deceptive code in master.

Thoughts?

~Andrew

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