| Message ID | 20190726135240.21745-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown | expand |
On Fri, Jul 26, 2019 at 02:52:39PM +0100, Andrew Cooper wrote: > Future changes are going to need to page align some percpu data. > > This means that the percpu area needs suiably aligning in the BSS so CPU0 has > correctly aligned data. Shuffle the exact link order of items within the BSS > to give .bss.percpu.page_aligned appropriate alignment. > > Additionally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > --- > xen/arch/arm/xen.lds.S | 5 +++-- > xen/arch/x86/xen.lds.S | 5 +++-- > xen/include/xen/percpu.h | 2 ++ > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 12c107f45d..07cbdf2543 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -201,14 +201,15 @@ SECTIONS > *(.bss.stack_aligned) > . = ALIGN(PAGE_SIZE); > *(.bss.page_aligned) > - *(.bss) > - . = ALIGN(SMP_CACHE_BYTES); Don't you also need a: . = ALIGN(PAGE_SIZE); here? Or is the size of .bss.page_aligned also aligned to page size? Thanks, Roger.
On 26/07/2019 15:22, Roger Pau Monné wrote: > On Fri, Jul 26, 2019 at 02:52:39PM +0100, Andrew Cooper wrote: >> Future changes are going to need to page align some percpu data. >> >> This means that the percpu area needs suiably aligning in the BSS so CPU0 has >> correctly aligned data. Shuffle the exact link order of items within the BSS >> to give .bss.percpu.page_aligned appropriate alignment. >> >> Additionally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wl@xen.org> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien.grall@arm.com> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> --- >> xen/arch/arm/xen.lds.S | 5 +++-- >> xen/arch/x86/xen.lds.S | 5 +++-- >> xen/include/xen/percpu.h | 2 ++ >> 3 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >> index 12c107f45d..07cbdf2543 100644 >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -201,14 +201,15 @@ SECTIONS >> *(.bss.stack_aligned) >> . = ALIGN(PAGE_SIZE); >> *(.bss.page_aligned) >> - *(.bss) >> - . = ALIGN(SMP_CACHE_BYTES); > Don't you also need a: > > . = ALIGN(PAGE_SIZE); > > here? No, (I don't think so). > Or is the size of .bss.page_aligned also aligned to page size? Every object inside .bss.page_aligned should have suitable (i.e. multiple of) size and alignment. Without this, things will break. ~Andrew
On 26.07.2019 16:30, Andrew Cooper wrote: > On 26/07/2019 15:22, Roger Pau Monné wrote: >> On Fri, Jul 26, 2019 at 02:52:39PM +0100, Andrew Cooper wrote: >>> Future changes are going to need to page align some percpu data. >>> >>> This means that the percpu area needs suiably aligning in the BSS so CPU0 has >>> correctly aligned data. Shuffle the exact link order of items within the BSS >>> to give .bss.percpu.page_aligned appropriate alignment. >>> >>> Additionally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Wei Liu <wl@xen.org> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Stefano Stabellini <sstabellini@kernel.org> >>> CC: Julien Grall <julien.grall@arm.com> >>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>> --- >>> xen/arch/arm/xen.lds.S | 5 +++-- >>> xen/arch/x86/xen.lds.S | 5 +++-- >>> xen/include/xen/percpu.h | 2 ++ >>> 3 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>> index 12c107f45d..07cbdf2543 100644 >>> --- a/xen/arch/arm/xen.lds.S >>> +++ b/xen/arch/arm/xen.lds.S >>> @@ -201,14 +201,15 @@ SECTIONS >>> *(.bss.stack_aligned) >>> . = ALIGN(PAGE_SIZE); >>> *(.bss.page_aligned) >>> - *(.bss) >>> - . = ALIGN(SMP_CACHE_BYTES); >> Don't you also need a: >> >> . = ALIGN(PAGE_SIZE); >> >> here? > > No, (I don't think so). > >> Or is the size of .bss.page_aligned also aligned to page size? > > Every object inside .bss.page_aligned should have suitable (i.e. > multiple of) size and alignment. Without this, things will break. I'm not sure we should have such a requirement: Objects in .*.page_aligned sections should themselves have PAGE_SIZE alignment (i.e. there shouldn't be a need to ALIGN() _ahead_ of the section directive in the script (that is, the one in context above should actually be redundant). But I'm not sure about demanding their size to be a multiple of PAGE_SIZE - while C will guarantee this (and waste space in certain cases), assembly constructs could still be written such that the trailing unused space can be re-used. Otoh I agree with your virtual statement of this being a little fragile. Jan
On 26/07/2019 15:38, Jan Beulich wrote: > On 26.07.2019 16:30, Andrew Cooper wrote: >> On 26/07/2019 15:22, Roger Pau Monné wrote: >>> On Fri, Jul 26, 2019 at 02:52:39PM +0100, Andrew Cooper wrote: >>>> Future changes are going to need to page align some percpu data. >>>> >>>> This means that the percpu area needs suiably aligning in the BSS so CPU0 has >>>> correctly aligned data. Shuffle the exact link order of items within the BSS >>>> to give .bss.percpu.page_aligned appropriate alignment. >>>> >>>> Additionally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: Wei Liu <wl@xen.org> >>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>> CC: Stefano Stabellini <sstabellini@kernel.org> >>>> CC: Julien Grall <julien.grall@arm.com> >>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>> --- >>>> xen/arch/arm/xen.lds.S | 5 +++-- >>>> xen/arch/x86/xen.lds.S | 5 +++-- >>>> xen/include/xen/percpu.h | 2 ++ >>>> 3 files changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>>> index 12c107f45d..07cbdf2543 100644 >>>> --- a/xen/arch/arm/xen.lds.S >>>> +++ b/xen/arch/arm/xen.lds.S >>>> @@ -201,14 +201,15 @@ SECTIONS >>>> *(.bss.stack_aligned) >>>> . = ALIGN(PAGE_SIZE); >>>> *(.bss.page_aligned) >>>> - *(.bss) >>>> - . = ALIGN(SMP_CACHE_BYTES); >>> Don't you also need a: >>> >>> . = ALIGN(PAGE_SIZE); >>> >>> here? >> No, (I don't think so). >> >>> Or is the size of .bss.page_aligned also aligned to page size? >> Every object inside .bss.page_aligned should have suitable (i.e. >> multiple of) size and alignment. Without this, things will break. > I'm not sure we should have such a requirement: Objects in > .*.page_aligned sections should themselves have PAGE_SIZE alignment > (i.e. there shouldn't be a need to ALIGN() _ahead_ of the section > directive in the script (that is, the one in context above should > actually be redundant). But I'm not sure about demanding their > size to be a multiple of PAGE_SIZE - while C will guarantee this > (and waste space in certain cases), assembly constructs could still > be written such that the trailing unused space can be re-used. Otoh > I agree with your virtual statement of this being a little fragile. I suppose that all which really matters is that each object has page alignment, and that will cause things to properly laid out. Where this goes wrong is for an object which isn't a multiple of 4k, followed by an object which doesn't have 4k alignment. Either way, I think the patch is fine as is. ~Andrew
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 12c107f45d..07cbdf2543 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -201,14 +201,15 @@ SECTIONS *(.bss.stack_aligned) . = ALIGN(PAGE_SIZE); *(.bss.page_aligned) - *(.bss) - . = ALIGN(SMP_CACHE_BYTES); __per_cpu_start = .; + *(.bss.percpu.page_aligned) *(.bss.percpu) . = ALIGN(SMP_CACHE_BYTES); *(.bss.percpu.read_mostly) . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; + *(.bss) + . = ALIGN(SMP_CACHE_BYTES); __bss_end = .; } :text _end = . ; diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index a73139cd29..b8a2ea4259 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -293,14 +293,15 @@ SECTIONS __bss_start = .; *(.bss.stack_aligned) *(.bss.page_aligned*) - *(.bss) - . = ALIGN(SMP_CACHE_BYTES); __per_cpu_start = .; + *(.bss.percpu.page_aligned) *(.bss.percpu) . = ALIGN(SMP_CACHE_BYTES); *(.bss.percpu.read_mostly) . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; + *(.bss) + . = ALIGN(SMP_CACHE_BYTES); __bss_end = .; } :text _end = . ; diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h index aeec5c19d6..c5291dc5e9 100644 --- a/xen/include/xen/percpu.h +++ b/xen/include/xen/percpu.h @@ -10,6 +10,8 @@ * macro expanded, while still allowing a per-architecture symbol name prefix. */ #define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, ) +#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \ + __DEFINE_PER_CPU(type, _##name, .page_aligned) #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \ __DEFINE_PER_CPU(type, _##name, .read_mostly)
Future changes are going to need to page align some percpu data. This means that the percpu area needs suiably aligning in the BSS so CPU0 has correctly aligned data. Shuffle the exact link order of items within the BSS to give .bss.percpu.page_aligned appropriate alignment. Additionally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> --- xen/arch/arm/xen.lds.S | 5 +++-- xen/arch/x86/xen.lds.S | 5 +++-- xen/include/xen/percpu.h | 2 ++ 3 files changed, 8 insertions(+), 4 deletions(-)