[RFC,1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata

Message ID c85903a7cfad37d14a7e5a4df9fc7119a3669fb3.1689130310.git.houwenlong.hwl@antgroup.com
State New
Headers
Series [RFC,1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata |

Commit Message

Hou Wenlong July 12, 2023, 3:30 a.m. UTC
  As startup_gdt and startup_gdt_descr are only used in booting, make them
as __initdata to allow them to be freed after boot.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kernel/head64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Comments

Ingo Molnar Oct. 16, 2023, 11:57 a.m. UTC | #1
* Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:

> As startup_gdt and startup_gdt_descr are only used in booting, make them
> as __initdata to allow them to be freed after boot.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  arch/x86/kernel/head64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index 49f7629b17f7..dd357807ffb5 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -69,7 +69,7 @@ EXPORT_SYMBOL(vmemmap_base);
>  /*
>   * GDT used on the boot CPU before switching to virtual addresses.
>   */
> -static struct desc_struct startup_gdt[GDT_ENTRIES] = {
> +static struct desc_struct startup_gdt[GDT_ENTRIES] __initdata = {
>  	[GDT_ENTRY_KERNEL32_CS]         = GDT_ENTRY_INIT(0xc09b, 0, 0xfffff),
>  	[GDT_ENTRY_KERNEL_CS]           = GDT_ENTRY_INIT(0xa09b, 0, 0xfffff),
>  	[GDT_ENTRY_KERNEL_DS]           = GDT_ENTRY_INIT(0xc093, 0, 0xfffff),
> @@ -79,7 +79,7 @@ static struct desc_struct startup_gdt[GDT_ENTRIES] = {
>   * Address needs to be set at runtime because it references the startup_gdt
>   * while the kernel still uses a direct mapping.
>   */
> -static struct desc_ptr startup_gdt_descr = {
> +static struct desc_ptr startup_gdt_descr __initdata = {
>  	.size = sizeof(startup_gdt),
>  	.address = 0,

Yeah, so I think this and patch #1 are independently useful of the PIE 
feature, and I merged them into tip:x86/boot for a v6.7 merge.

Mind re-sending any other patches for x86 that can be merged? For example 
patch #6 looks good too, but was mixed up a bit with a previous 
PIE-enablement patch. Moving the __head definition to a header should also 
probably done as a separate patch, followed by the widening of its use.

Thanks,

	Ingo
  
Hou Wenlong Oct. 17, 2023, 7:23 a.m. UTC | #2
On Mon, Oct 16, 2023 at 07:57:06PM +0800, Ingo Molnar wrote:
> 
> * Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
> 
> > As startup_gdt and startup_gdt_descr are only used in booting, make them
> > as __initdata to allow them to be freed after boot.
> > 
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> > ---
> >  arch/x86/kernel/head64.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> > index 49f7629b17f7..dd357807ffb5 100644
> > --- a/arch/x86/kernel/head64.c
> > +++ b/arch/x86/kernel/head64.c
> > @@ -69,7 +69,7 @@ EXPORT_SYMBOL(vmemmap_base);
> >  /*
> >   * GDT used on the boot CPU before switching to virtual addresses.
> >   */
> > -static struct desc_struct startup_gdt[GDT_ENTRIES] = {
> > +static struct desc_struct startup_gdt[GDT_ENTRIES] __initdata = {
> >  	[GDT_ENTRY_KERNEL32_CS]         = GDT_ENTRY_INIT(0xc09b, 0, 0xfffff),
> >  	[GDT_ENTRY_KERNEL_CS]           = GDT_ENTRY_INIT(0xa09b, 0, 0xfffff),
> >  	[GDT_ENTRY_KERNEL_DS]           = GDT_ENTRY_INIT(0xc093, 0, 0xfffff),
> > @@ -79,7 +79,7 @@ static struct desc_struct startup_gdt[GDT_ENTRIES] = {
> >   * Address needs to be set at runtime because it references the startup_gdt
> >   * while the kernel still uses a direct mapping.
> >   */
> > -static struct desc_ptr startup_gdt_descr = {
> > +static struct desc_ptr startup_gdt_descr __initdata = {
> >  	.size = sizeof(startup_gdt),
> >  	.address = 0,
> 
> Yeah, so I think this and patch #1 are independently useful of the PIE 
> feature, and I merged them into tip:x86/boot for a v6.7 merge.
> 
> Mind re-sending any other patches for x86 that can be merged? For example 
> patch #6 looks good too, but was mixed up a bit with a previous 
> PIE-enablement patch. Moving the __head definition to a header should also 
> probably done as a separate patch, followed by the widening of its use.
>
Hi Ingo,

I have sent patch #6 separately for x86. Do you have any ideas about
building the head code as PIE? Should I resend the patchset for the PIE
feature?

Thanks!

> Thanks,
> 
> 	Ingo
  
Ingo Molnar Oct. 17, 2023, 1:02 p.m. UTC | #3
* Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:

> Hi Ingo,
> 
> I have sent patch #6 separately for x86. Do you have any ideas about 
> building the head code as PIE? Should I resend the patchset for the PIE 
> feature?

So I had a brief look, and despite reading 0/43 it was unclear to me what 
the precise advantages of building as PIE are.

Ie. could you please outline:

 - *Exactly* how much PIE based KASLR randomization would gain us in terms 
   of randomization granularity and effective number of randomization bits, 
   compared to the current status quo?

 - How is code generation changed at the instruction level - how does 
   kernel size change and what are the micro-advantages/disadvantages?

 - Are there any other advantages/motivation than improving KASLR?

Ie. before asking us to apply ~50 patches and add a whole new build mode 
and the maintainance overhead to support it into infinity and beyond, could 
you please offer a better list of pros and cons?

Thanks,

	Ingo
  
H. Peter Anvin Oct. 17, 2023, 4:34 p.m. UTC | #4
On October 17, 2023 6:02:27 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
>
>> Hi Ingo,
>> 
>> I have sent patch #6 separately for x86. Do you have any ideas about 
>> building the head code as PIE? Should I resend the patchset for the PIE 
>> feature?
>
>So I had a brief look, and despite reading 0/43 it was unclear to me what 
>the precise advantages of building as PIE are.
>
>Ie. could you please outline:
>
> - *Exactly* how much PIE based KASLR randomization would gain us in terms 
>   of randomization granularity and effective number of randomization bits, 
>   compared to the current status quo?
>
> - How is code generation changed at the instruction level - how does 
>   kernel size change and what are the micro-advantages/disadvantages?
>
> - Are there any other advantages/motivation than improving KASLR?
>
>Ie. before asking us to apply ~50 patches and add a whole new build mode 
>and the maintainance overhead to support it into infinity and beyond, could 
>you please offer a better list of pros and cons?
>
>Thanks,
>
>	Ingo

If the goal is better KASLR, then what we really should spend time on was Kristen Accardi's fgKASLR patches, which not only exponentially(!) increases the randomization entrophy but also *actually* avoids the "one leak and it's over" problem.

However, she gave up on it because she got no interest, despite working code.
  
Hou Wenlong Oct. 18, 2023, 8:36 a.m. UTC | #5
On Tue, Oct 17, 2023 at 09:02:27PM +0800, Ingo Molnar wrote:
> 
> * Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
> 
> > Hi Ingo,
> > 
> > I have sent patch #6 separately for x86. Do you have any ideas about 
> > building the head code as PIE? Should I resend the patchset for the PIE 
> > feature?
> 
> So I had a brief look, and despite reading 0/43 it was unclear to me what 
> the precise advantages of building as PIE are.
>
> Ie. could you please outline:
> 
>  - *Exactly* how much PIE based KASLR randomization would gain us in terms 
>    of randomization granularity and effective number of randomization bits, 
>    compared to the current status quo?
> 
>  - How is code generation changed at the instruction level - how does 
>    kernel size change and what are the micro-advantages/disadvantages?
> 
>  - Are there any other advantages/motivation than improving KASLR?
> 
> Ie. before asking us to apply ~50 patches and add a whole new build mode 
> and the maintainance overhead to support it into infinity and beyond, could 
> you please offer a better list of pros and cons?
> 
Hi Ingo,

Thanks for your reply. I apologize for the confusion. Waht I meant to
say is that I would like to resend the remaining part of this patchset
that building the head code as PIE. As mentioned in the cover letter,
building the head code as PIE can eliminate certain workarounds such as
the "fixup_pointer" in head64.c and the inline assembly in
mem_encrypt_identity.c. This is considered a cleanup. However, it is
still necessary to use inline assembly to obtain the absolute symbol
value during the pagetable building process.

Regarding the entire PIE patchset, I agree that it is complex and there
are no obvious use cases apart from improving KASLR. As mentioned
earlier, the primary motivation is to increase the flexibility of the
kernel image address rather than prioritizing security, enabling the
kernel image to be placed at any virtual address. The use cases in our
internal environment are specific and not widespread, so we do not feel
an urgent need to push it forward at the moment.

Thanks!

> Thanks,
> 
> 	Ingo
  
Ingo Molnar Oct. 18, 2023, 11:45 a.m. UTC | #6
* H. Peter Anvin <hpa@zytor.com> wrote:

> If the goal is better KASLR, then what we really should spend time on was 
> Kristen Accardi's fgKASLR patches, which not only exponentially(!) 
> increases the randomization entrophy but also *actually* avoids the "one 
> leak and it's over" problem.

Agreed. Going by this version of function-granularity KASLR from 3 years 
ago:

  https://lwn.net/Articles/824307/
  https://lwn.net/ml/linux-kernel/20200623172327.5701-1-kristen@linux.intel.com/

The fgKASLR feature looks entirely viable to me. Back then I presumed it 
would get iterated beyond v3, and then it fell off my radar. :-/

If Kristen or someone else would like to dust this off & submit a fresh 
version it would be much appreciated!

Thanks,

	Ingo
  
Josh Poimboeuf Nov. 10, 2023, 12:03 a.m. UTC | #7
On Wed, Oct 18, 2023 at 01:45:40PM +0200, Ingo Molnar wrote:
> 
> * H. Peter Anvin <hpa@zytor.com> wrote:
> 
> > If the goal is better KASLR, then what we really should spend time on was 
> > Kristen Accardi's fgKASLR patches, which not only exponentially(!) 
> > increases the randomization entrophy but also *actually* avoids the "one 
> > leak and it's over" problem.
> 
> Agreed. Going by this version of function-granularity KASLR from 3 years 
> ago:
> 
>   https://lwn.net/Articles/824307/
>   https://lwn.net/ml/linux-kernel/20200623172327.5701-1-kristen@linux.intel.com/
> 
> The fgKASLR feature looks entirely viable to me. Back then I presumed it 
> would get iterated beyond v3, and then it fell off my radar. :-/
> 
> If Kristen or someone else would like to dust this off & submit a fresh 
> version it would be much appreciated!

That actually got up to v10:

  https://lkml.kernel.org/lkml/20220209185752.1226407-1-alexandr.lobakin@intel.com

Anyway, I'm also very interested in this.  If nobody else is working on
it then I could give it a try.

BTW I've wondered if translation-unit granularity would be more
preferable than function granularity due to improved i-cache locality
and (possibly) easier livepatch compatibility.
  

Patch

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 49f7629b17f7..dd357807ffb5 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -69,7 +69,7 @@  EXPORT_SYMBOL(vmemmap_base);
 /*
  * GDT used on the boot CPU before switching to virtual addresses.
  */
-static struct desc_struct startup_gdt[GDT_ENTRIES] = {
+static struct desc_struct startup_gdt[GDT_ENTRIES] __initdata = {
 	[GDT_ENTRY_KERNEL32_CS]         = GDT_ENTRY_INIT(0xc09b, 0, 0xfffff),
 	[GDT_ENTRY_KERNEL_CS]           = GDT_ENTRY_INIT(0xa09b, 0, 0xfffff),
 	[GDT_ENTRY_KERNEL_DS]           = GDT_ENTRY_INIT(0xc093, 0, 0xfffff),
@@ -79,7 +79,7 @@  static struct desc_struct startup_gdt[GDT_ENTRIES] = {
  * Address needs to be set at runtime because it references the startup_gdt
  * while the kernel still uses a direct mapping.
  */
-static struct desc_ptr startup_gdt_descr = {
+static struct desc_ptr startup_gdt_descr __initdata = {
 	.size = sizeof(startup_gdt),
 	.address = 0,
 };