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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2019 16:59, Andrew Cooper wrote:
> 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.
Will do.

> > 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.
It would be nice to be able to say

#define get_per_cpu_var(init_tss) get_per_cpu_var(init_tss_page.tss)

>   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?
I have no problem at all dropping the init_ here. It's not clear to me
though whether you saying "I'm not happy with introducing this diversion"
implies anything further that you'd want or even expect me to change.

Jan

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