diff mbox series

[Question] Should retain 2M alignment if kernel image is bad aligned at entry or BSS overlaps?

Message ID 894d1598-7b05-9406-5c1a-162b749bb4e8@huawei.com
State New
Headers show
Series [Question] Should retain 2M alignment if kernel image is bad aligned at entry or BSS overlaps? | expand

Commit Message

Kefeng Wang March 1, 2022, 6:49 a.m. UTC
Hi Ard,Will and all maintainer,

As commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again
for relocatable kernels") saids, a relocatable image does not need to
be copied to a 2M aligned offset if it was loaded on a 64k boundary
(for a 4 KB granule kernel) by EFI. But if there is some FIRMWARE BUG,

1) kernel image not aligned on 64k boundary
or
2) Image BSS overlaps adjacent EFI memory region

When kaslr is disabled(eg, EFI_RNG_PROTOCOL unavailable), it will leads
KPTI forced ON after kernel image relocated, message shown below,

   CPU features: kernel page table isolation forced ON by KASLR
   ...
   KASLR disabled due to lack of seed

The KASLR don't enabled actually, and KPTI is forced enabled which could
affect performance.

I found commit 7c116db24d94 ("efi/libstub/arm64: Retain 2MB kernel Image
alignment if !KASLR") in v5.8, should we retain 2M alignment if kernel image
is bad aligned at entry or BSS overlaps?


         /*
@@ -119,9 +120,11 @@ efi_status_t handle_kernel_image(unsigned long 
*image_addr,
         if (image->image_base != _text)
                 efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base 
has bogus value\n");

-       if (!IS_ALIGNED((u64)_text, SEGMENT_ALIGN))
+       if (!IS_ALIGNED((u64)_text, SEGMENT_ALIGN)) {
+               need_2m_aligned = true;
                 efi_err("FIRMWARE BUG: kernel image not aligned on %dk 
boundary\n",
                         SEGMENT_ALIGN >> 10);
+       }

         kernel_size = _edata - _text;
         kernel_memsize = kernel_size + (_end - _edata);
@@ -142,6 +145,7 @@ efi_status_t handle_kernel_image(unsigned long 
*image_addr,

         if (status != EFI_SUCCESS) {
                 if (!check_image_region((u64)_text, kernel_memsize)) {
+                       need_2m_aligned = true;
                         efi_err("FIRMWARE BUG: Image BSS overlaps 
adjacent EFI memory region\n");
                 } else if (IS_ALIGNED((u64)_text, min_kimg_align)) {
                         /*
@@ -152,7 +156,8 @@ efi_status_t handle_kernel_image(unsigned long 
*image_addr,
                         *reserve_size = 0;
                         return EFI_SUCCESS;
                 }
-
+               if (efi_nokaslr & need_2m_aligned)
+                       min_kimg_align = MIN_KIMG_ALIGN;
                 status = efi_allocate_pages_aligned(*reserve_size, 
reserve_addr,
                                                     ULONG_MAX, 
min_kimg_align);

Comments

Kefeng Wang March 1, 2022, 10:34 a.m. UTC | #1
On 2022/3/1 15:22, Ard Biesheuvel wrote:
> On Tue, 1 Mar 2022 at 07:50, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>> Hi Ard,Will and all maintainer,
>>
>> As commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again
>> for relocatable kernels") saids, a relocatable image does not need to
>> be copied to a 2M aligned offset if it was loaded on a 64k boundary
>> (for a 4 KB granule kernel) by EFI. But if there is some FIRMWARE BUG,
>>
>> 1) kernel image not aligned on 64k boundary
>> or
>> 2) Image BSS overlaps adjacent EFI memory region
>>
>> When kaslr is disabled(eg, EFI_RNG_PROTOCOL unavailable), it will leads
>> KPTI forced ON after kernel image relocated, message shown below,
>>
>>     CPU features: kernel page table isolation forced ON by KASLR
>>     ...
>>     KASLR disabled due to lack of seed
>>
>> The KASLR don't enabled actually, and KPTI is forced enabled which could
>> affect performance.
>>
> This message is misleading. If the alignment modulo 2M != 0, we still
> have 5 bits of 'randomization', although these bits are probably
> highly predictable on a given system.
Yes, this kernel boot message is misleading, I am confused and find
commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again for
relocatable kernels") leads to different behavior about KPTI.
>> I found commit 7c116db24d94 ("efi/libstub/arm64: Retain 2MB kernel Image
>> alignment if !KASLR") in v5.8, should we retain 2M alignment if kernel image
>> is bad aligned at entry or BSS overlaps?
>>
> Personally, I think we're doing enough already to deal with Redhat's
> broken out-of-tree GRUB/SHIM concoction, which is the primary reason
> for these workarounds  IIRC.

Not sure about this, what's your mean is that error message is enough and

no need to adjust the alignment when image with bad aligned at entry or 
BSS overlaps?

> You can already pass nokalsr on the kernel command line if you want to
> avoid the downsides entirely, so as I understand it, this is mostly
> about an unquantified performance gain on systems that use a broken
> bootloader and lack the entropy source for a KASLR seed, but are not
> able to put nokaslr on the command line?

nokaslr will use 2M alignment by default, but if some board with new 
BIOS/GRUB

the kaslr won't enabled unless change the grub to drop it one by one, it 
is not kind

for production deployment.

Do you think the following adjustment make sense or it is definitely wrong?

Any other option, thanks for your feedback.


>
>
>> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c
>> b/drivers/firmware/efi/libstub/arm64-stub.c
>> index 9cc556013d08..47ecd9b80db3 100644
>> --- a/drivers/firmware/efi/libstub/arm64-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
>> @@ -87,6 +87,7 @@ efi_status_t handle_kernel_image(unsigned long
>> *image_addr,
>>    {
>>           efi_status_t status;
>>           unsigned long kernel_size, kernel_memsize = 0;
>> +       bool need_2m_aligned = false;
>>           u32 phys_seed = 0;
>>
>>           /*
>> @@ -119,9 +120,11 @@ efi_status_t handle_kernel_image(unsigned long
>> *image_addr,
>>           if (image->image_base != _text)
>>                   efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base
>> has bogus value\n");
>>
>> -       if (!IS_ALIGNED((u64)_text, SEGMENT_ALIGN))
>> +       if (!IS_ALIGNED((u64)_text, SEGMENT_ALIGN)) {
>> +               need_2m_aligned = true;
>>                   efi_err("FIRMWARE BUG: kernel image not aligned on %dk
>> boundary\n",
>>                           SEGMENT_ALIGN >> 10);
>> +       }
>>
>>           kernel_size = _edata - _text;
>>           kernel_memsize = kernel_size + (_end - _edata);
>> @@ -142,6 +145,7 @@ efi_status_t handle_kernel_image(unsigned long
>> *image_addr,
>>
>>           if (status != EFI_SUCCESS) {
>>                   if (!check_image_region((u64)_text, kernel_memsize)) {
>> +                       need_2m_aligned = true;
>>                           efi_err("FIRMWARE BUG: Image BSS overlaps
>> adjacent EFI memory region\n");
>>                   } else if (IS_ALIGNED((u64)_text, min_kimg_align)) {
>>                           /*
>> @@ -152,7 +156,8 @@ efi_status_t handle_kernel_image(unsigned long
>> *image_addr,
>>                           *reserve_size = 0;
>>                           return EFI_SUCCESS;
>>                   }
>> -
>> +               if (efi_nokaslr & need_2m_aligned)
>> +                       min_kimg_align = MIN_KIMG_ALIGN;
>>                   status = efi_allocate_pages_aligned(*reserve_size,
>> reserve_addr,
>>                                                       ULONG_MAX,
>> min_kimg_align);
> .
Ard Biesheuvel March 1, 2022, 12:57 p.m. UTC | #2
On Tue, 1 Mar 2022 at 11:34, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
> On 2022/3/1 15:22, Ard Biesheuvel wrote:
> > On Tue, 1 Mar 2022 at 07:50, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >> Hi Ard,Will and all maintainer,
> >>
> >> As commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again
> >> for relocatable kernels") saids, a relocatable image does not need to
> >> be copied to a 2M aligned offset if it was loaded on a 64k boundary
> >> (for a 4 KB granule kernel) by EFI. But if there is some FIRMWARE BUG,
> >>
> >> 1) kernel image not aligned on 64k boundary
> >> or
> >> 2) Image BSS overlaps adjacent EFI memory region
> >>
> >> When kaslr is disabled(eg, EFI_RNG_PROTOCOL unavailable), it will leads
> >> KPTI forced ON after kernel image relocated, message shown below,
> >>
> >>     CPU features: kernel page table isolation forced ON by KASLR
> >>     ...
> >>     KASLR disabled due to lack of seed
> >>
> >> The KASLR don't enabled actually, and KPTI is forced enabled which could
> >> affect performance.
> >>
> > This message is misleading. If the alignment modulo 2M != 0, we still
> > have 5 bits of 'randomization', although these bits are probably
> > highly predictable on a given system.
> Yes, this kernel boot message is misleading, I am confused and find
> commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again for
> relocatable kernels") leads to different behavior about KPTI.

That commit log explains how we ended up forcing 2M alignment
inadvertently if the EFI_RNG_PROTOCOL was not available. If we don't
force 2M alignment, and the physical address of the kernel happens to
be misaligned modulo 2M, the resulting offset is reused for virtual
randomization as well. Hence the 5 additional bits.

Given the predictability of those bits when used on their own, we
could decide to disable KPTI in this case as well.

> >> I found commit 7c116db24d94 ("efi/libstub/arm64: Retain 2MB kernel Image
> >> alignment if !KASLR") in v5.8, should we retain 2M alignment if kernel image
> >> is bad aligned at entry or BSS overlaps?
> >>
> > Personally, I think we're doing enough already to deal with Redhat's
> > broken out-of-tree GRUB/SHIM concoction, which is the primary reason
> > for these workarounds  IIRC.
>
> Not sure about this, what's your mean is that error message is enough and
>
> no need to adjust the alignment when image with bad aligned at entry or
> BSS overlaps?
>

I am having difficulty understanding which part of the current
behavior you think is causing a problem.

In a nutshell, what the current code aims to do is to only move the
image in memory if needed, and just boot if where it was loaded by EFI
otherwise.

Reasons for moving it could be any of:
- KASLR is enabled and we have a seed
- nokaslr was passed, and we are not aligned to 2M
- we are running a 64k pages + VMAP stack build, and the image is not
aligned to 128k
- the PE/COFF loader is broken, and ignored the minimum 64k alignment
in the PE/COFF header
- the PE/COFF loader is broken, and ignored the BSS size, resulting in
an overlap with a memory region that is already in use.

As you might have guessed, my grumbling about GRUB/SHIM was in
relation to the latter 2 points - upstream GRUB does not have its own
PE/COFF loader, but SHIM/GRUB implement their own, and don't follow
the PE/COFF spec too rigorously.

> > You can already pass nokalsr on the kernel command line if you want to
> > avoid the downsides entirely, so as I understand it, this is mostly
> > about an unquantified performance gain on systems that use a broken
> > bootloader and lack the entropy source for a KASLR seed, but are not
> > able to put nokaslr on the command line?
>
> nokaslr will use 2M alignment by default, but if some board with new
> BIOS/GRUB
>
> the kaslr won't enabled unless change the grub to drop it one by one, it
> is not kind
>
> for production deployment.
>
> Do you think the following adjustment make sense or it is definitely wrong?
>

I can only answer this if I understand which problem it solves. Why do
you need the 2M alignment in this case?
Kefeng Wang March 3, 2022, 6:14 a.m. UTC | #3
On 2022/3/1 20:57, Ard Biesheuvel wrote:
> On Tue, 1 Mar 2022 at 11:34, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>> On 2022/3/1 15:22, Ard Biesheuvel wrote:
>>> On Tue, 1 Mar 2022 at 07:50, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>> Hi Ard,Will and all maintainer,
>>>>
>>>> As commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again
>>>> for relocatable kernels") saids, a relocatable image does not need to
>>>> be copied to a 2M aligned offset if it was loaded on a 64k boundary
>>>> (for a 4 KB granule kernel) by EFI. But if there is some FIRMWARE BUG,
>>>>
>>>> 1) kernel image not aligned on 64k boundary
>>>> or
>>>> 2) Image BSS overlaps adjacent EFI memory region
>>>>
>>>> When kaslr is disabled(eg, EFI_RNG_PROTOCOL unavailable), it will leads
>>>> KPTI forced ON after kernel image relocated, message shown below,
>>>>
>>>>      CPU features: kernel page table isolation forced ON by KASLR
>>>>      ...
>>>>      KASLR disabled due to lack of seed
>>>>
>>>> The KASLR don't enabled actually, and KPTI is forced enabled which could
>>>> affect performance.
>>>>
>>> This message is misleading. If the alignment modulo 2M != 0, we still
>>> have 5 bits of 'randomization', although these bits are probably
>>> highly predictable on a given system.
>> Yes, this kernel boot message is misleading, I am confused and find
>> commit 3a262423755b ("efi/libstub: arm64: Relax 2M alignment again for
>> relocatable kernels") leads to different behavior about KPTI.
> That commit log explains how we ended up forcing 2M alignment
> inadvertently if the EFI_RNG_PROTOCOL was not available. If we don't
> force 2M alignment, and the physical address of the kernel happens to
> be misaligned modulo 2M, the resulting offset is reused for virtual
> randomization as well. Hence the 5 additional bits.
>
> Given the predictability of those bits when used on their own, we
> could decide to disable KPTI in this case as well.
>
>>>> I found commit 7c116db24d94 ("efi/libstub/arm64: Retain 2MB kernel Image
>>>> alignment if !KASLR") in v5.8, should we retain 2M alignment if kernel image
>>>> is bad aligned at entry or BSS overlaps?
>>>>
>>> Personally, I think we're doing enough already to deal with Redhat's
>>> broken out-of-tree GRUB/SHIM concoction, which is the primary reason
>>> for these workarounds  IIRC.
>> Not sure about this, what's your mean is that error message is enough and
>>
>> no need to adjust the alignment when image with bad aligned at entry or
>> BSS overlaps?
>>
> I am having difficulty understanding which part of the current
> behavior you think is causing a problem.
>
> In a nutshell, what the current code aims to do is to only move the
> image in memory if needed, and just boot if where it was loaded by EFI
> otherwise.
>
> Reasons for moving it could be any of:
> - KASLR is enabled and we have a seed
> - nokaslr was passed, and we are not aligned to 2M
> - we are running a 64k pages + VMAP stack build, and the image is not
> aligned to 128k
> - the PE/COFF loader is broken, and ignored the minimum 64k alignment
> in the PE/COFF header
> - the PE/COFF loader is broken, and ignored the BSS size, resulting in
> an overlap with a memory region that is already in use.
>
> As you might have guessed, my grumbling about GRUB/SHIM was in
> relation to the latter 2 points - upstream GRUB does not have its own
> PE/COFF loader, but SHIM/GRUB implement their own, and don't follow
> the PE/COFF spec too rigorously.
>
>>> You can already pass nokalsr on the kernel command line if you want to
>>> avoid the downsides entirely, so as I understand it, this is mostly
>>> about an unquantified performance gain on systems that use a broken
>>> bootloader and lack the entropy source for a KASLR seed, but are not
>>> able to put nokaslr on the command line?
>> nokaslr will use 2M alignment by default, but if some board with new
>> BIOS/GRUB
>>
>> the kaslr won't enabled unless change the grub to drop it one by one, it
>> is not kind
>>
>> for production deployment.
>>
>> Do you think the following adjustment make sense or it is definitely wrong?
>>
> I can only answer this if I understand which problem it solves. Why do
> you need the 2M alignment in this case?
> .

Sorry for the late response,my purpose is that we don't want to enable 
KPTI if

KASLR is disabled. For now, if there are some firmware bug, the kernel 
image is

relocated which lead to kaslr_requires_kpti() returen 
ture(kaslr_offset() > 0).

the change to 2M alignment is a workaround and according to your 
explanation,

I don't think the workaround is necessary.  I want to make sure that the 
above

scene is expected? thanks.
Ard Biesheuvel March 3, 2022, 11:31 a.m. UTC | #4
On Thu, 3 Mar 2022 at 06:14, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
> On 2022/3/1 20:57, Ard Biesheuvel wrote:
> > On Tue, 1 Mar 2022 at 11:34, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >>
...
> >> Do you think the following adjustment make sense or it is definitely wrong?
> >>
> > I can only answer this if I understand which problem it solves. Why do
> > you need the 2M alignment in this case?
> > .
>
> Sorry for the late response,my purpose is that we don't want to enable
> KPTI if
>
> KASLR is disabled. For now, if there are some firmware bug, the kernel
> image is
>
> relocated which lead to kaslr_requires_kpti() returen
> ture(kaslr_offset() > 0).
>
> the change to 2M alignment is a workaround and according to your
> explanation,
>
> I don't think the workaround is necessary.  I want to make sure that the
> above
>
> scene is expected? thanks.
>

I don't think we need this.
Kefeng Wang March 3, 2022, 4:03 p.m. UTC | #5
On 2022/3/3 19:31, Ard Biesheuvel wrote:
> On Thu, 3 Mar 2022 at 06:14, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>> On 2022/3/1 20:57, Ard Biesheuvel wrote:
>>> On Tue, 1 Mar 2022 at 11:34, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> ...
>>>> Do you think the following adjustment make sense or it is definitely wrong?
>>>>
>>> I can only answer this if I understand which problem it solves. Why do
>>> you need the 2M alignment in this case?
>>> .
>> Sorry for the late response,my purpose is that we don't want to enable
>> KPTI if
>>
>> KASLR is disabled. For now, if there are some firmware bug, the kernel
>> image is
>>
>> relocated which lead to kaslr_requires_kpti() returen
>> ture(kaslr_offset() > 0).
>>
>> the change to 2M alignment is a workaround and according to your
>> explanation,
>>
>> I don't think the workaround is necessary.  I want to make sure that the
>> above
>>
>> scene is expected? thanks.
>>
> I don't think we need this.
Got it, thanks.
> .
diff mbox series

Patch

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c 
b/drivers/firmware/efi/libstub/arm64-stub.c
index 9cc556013d08..47ecd9b80db3 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -87,6 +87,7 @@  efi_status_t handle_kernel_image(unsigned long 
*image_addr,
  {
         efi_status_t status;
         unsigned long kernel_size, kernel_memsize = 0;
+       bool need_2m_aligned = false;
         u32 phys_seed = 0;