2023-07-12 03:46:13

by Hou Wenlong

[permalink] [raw]
Subject: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata

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 <[email protected]>
---
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,
};
--
2.31.1



2023-10-16 11:57:17

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata


* Hou Wenlong <[email protected]> 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 <[email protected]>
> ---
> 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

2023-10-17 07:23:37

by Hou Wenlong

[permalink] [raw]
Subject: Re: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata

On Mon, Oct 16, 2023 at 07:57:06PM +0800, Ingo Molnar wrote:
>
> * Hou Wenlong <[email protected]> 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 <[email protected]>
> > ---
> > 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

2023-10-17 13:02:38

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata


* Hou Wenlong <[email protected]> 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

2023-10-17 17:11:03

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata

On October 17, 2023 6:02:27 AM PDT, Ingo Molnar <[email protected]> wrote:
>
>* Hou Wenlong <[email protected]> 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.

2023-10-18 08:36:59

by Hou Wenlong

[permalink] [raw]
Subject: Re: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata

On Tue, Oct 17, 2023 at 09:02:27PM +0800, Ingo Molnar wrote:
>
> * Hou Wenlong <[email protected]> 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

2023-10-18 11:46:24

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH RFC 1/7] x86/head/64: Mark startup_gdt and startup_gdt_descr as __initdata


* H. Peter Anvin <[email protected]> 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/[email protected]/

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