diff mbox series

efi: stub: prefer mirrored memory for randomized allocations

Message ID 20220503152131.263711-1-ardb@kernel.org
State Accepted
Commit a6cfe03c34bad8c7f51aba49a73403e348c51d1f
Headers show
Series efi: stub: prefer mirrored memory for randomized allocations | expand

Commit Message

Ard Biesheuvel May 3, 2022, 3:21 p.m. UTC
If the system exposes memory regions with the EFI_MORE_RELIABLE
attribute, it is implied that it is intended to be used for allocations
that are relatively important, such as the kernel's static image.

Since efi_random_alloc() is mostly (only) used for allocating space for
the kernel image, let's update it to take this into account, and
disregard all memory without the EFI_MORE_RELIABLE attribute if there is
sufficient memory available that does have this attribute.

Note that this change only affects booting with randomization enabled.
In other cases, the EFI stub runs the kernel image in place unless its
placement is unsuitable for some reason (i.e., misaligned, or its BSS
overlaps with another allocation), and it is left to the bootloader to
ensure that the kernel was loaded into EFI_MORE_RELIABLE memory if this
is desired.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/randomalloc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Kefeng Wang May 5, 2022, 1:43 p.m. UTC | #1
On 2022/5/3 23:21, Ard Biesheuvel wrote:
> If the system exposes memory regions with the EFI_MORE_RELIABLE
> attribute, it is implied that it is intended to be used for allocations
> that are relatively important, such as the kernel's static image.
>
> Since efi_random_alloc() is mostly (only) used for allocating space for
> the kernel image, let's update it to take this into account, and
> disregard all memory without the EFI_MORE_RELIABLE attribute if there is
> sufficient memory available that does have this attribute.
>
> Note that this change only affects booting with randomization enabled.
> In other cases, the EFI stub runs the kernel image in place unless its
> placement is unsuitable for some reason (i.e., misaligned, or its BSS
> overlaps with another allocation), and it is left to the bootloader to
> ensure that the kernel was loaded into EFI_MORE_RELIABLE memory if this
> is desired.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   drivers/firmware/efi/libstub/randomalloc.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
> index 724155b9e10d..07a762910312 100644
> --- a/drivers/firmware/efi/libstub/randomalloc.c
> +++ b/drivers/firmware/efi/libstub/randomalloc.c
> @@ -56,6 +56,7 @@ efi_status_t efi_random_alloc(unsigned long size,
>   			      unsigned long random_seed)
>   {
>   	unsigned long map_size, desc_size, total_slots = 0, target_slot;
> +	unsigned long total_mirrored_slots = 0;
>   	unsigned long buff_size;
>   	efi_status_t status;
>   	efi_memory_desc_t *memory_map;
> @@ -86,8 +87,14 @@ efi_status_t efi_random_alloc(unsigned long size,
>   		slots = get_entry_num_slots(md, size, ilog2(align));
>   		MD_NUM_SLOTS(md) = slots;
>   		total_slots += slots;
> +		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> +			total_mirrored_slots += slots;
>   	}
>   
> +	/* only consider mirrored slots for randomization if any exist */
> +	if (total_mirrored_slots > 0)
> +		total_slots = total_mirrored_slots;
> +

The kernel will check 4G lower limit to enable kernelcore=mirror feature.

Do we need some fallback mechanism in case of small mirror slots which

leads to fail allocation for Image?


>   	/* find a random number between 0 and total_slots */
>   	target_slot = (total_slots * (u64)(random_seed & U32_MAX)) >> 32;
>   
> @@ -107,6 +114,10 @@ efi_status_t efi_random_alloc(unsigned long size,
>   		efi_physical_addr_t target;
>   		unsigned long pages;
>   
> +		if (total_mirrored_slots > 0 &&
> +		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
> +			continue;
> +
>   		if (target_slot >= MD_NUM_SLOTS(md)) {
>   			target_slot -= MD_NUM_SLOTS(md);
>   			continue;
Ard Biesheuvel May 5, 2022, 4:12 p.m. UTC | #2
On Thu, 5 May 2022 at 15:43, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
> On 2022/5/3 23:21, Ard Biesheuvel wrote:
> > If the system exposes memory regions with the EFI_MORE_RELIABLE
> > attribute, it is implied that it is intended to be used for allocations
> > that are relatively important, such as the kernel's static image.
> >
> > Since efi_random_alloc() is mostly (only) used for allocating space for
> > the kernel image, let's update it to take this into account, and
> > disregard all memory without the EFI_MORE_RELIABLE attribute if there is
> > sufficient memory available that does have this attribute.
> >
> > Note that this change only affects booting with randomization enabled.
> > In other cases, the EFI stub runs the kernel image in place unless its
> > placement is unsuitable for some reason (i.e., misaligned, or its BSS
> > overlaps with another allocation), and it is left to the bootloader to
> > ensure that the kernel was loaded into EFI_MORE_RELIABLE memory if this
> > is desired.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   drivers/firmware/efi/libstub/randomalloc.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
> > index 724155b9e10d..07a762910312 100644
> > --- a/drivers/firmware/efi/libstub/randomalloc.c
> > +++ b/drivers/firmware/efi/libstub/randomalloc.c
> > @@ -56,6 +56,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> >                             unsigned long random_seed)
> >   {
> >       unsigned long map_size, desc_size, total_slots = 0, target_slot;
> > +     unsigned long total_mirrored_slots = 0;
> >       unsigned long buff_size;
> >       efi_status_t status;
> >       efi_memory_desc_t *memory_map;
> > @@ -86,8 +87,14 @@ efi_status_t efi_random_alloc(unsigned long size,
> >               slots = get_entry_num_slots(md, size, ilog2(align));
> >               MD_NUM_SLOTS(md) = slots;
> >               total_slots += slots;
> > +             if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> > +                     total_mirrored_slots += slots;
> >       }
> >
> > +     /* only consider mirrored slots for randomization if any exist */
> > +     if (total_mirrored_slots > 0)
> > +             total_slots = total_mirrored_slots;
> > +
>
> The kernel will check 4G lower limit to enable kernelcore=mirror feature.
>

Why? I mean, why is 4G a magic number also on arm64?


> Do we need some fallback mechanism in case of small mirror slots which
>
> leads to fail allocation for Image?
>

This code only counts slots that are large enough to hold the Image so
this can never happen. If total_mirrored_slots > 0, there is at least
one possible placement of the kernel where it falls entirely inside a
EFI_MORE_RELIABLE region.

>
> >       /* find a random number between 0 and total_slots */
> >       target_slot = (total_slots * (u64)(random_seed & U32_MAX)) >> 32;
> >
> > @@ -107,6 +114,10 @@ efi_status_t efi_random_alloc(unsigned long size,
> >               efi_physical_addr_t target;
> >               unsigned long pages;
> >
> > +             if (total_mirrored_slots > 0 &&
> > +                 !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
> > +                     continue;
> > +
> >               if (target_slot >= MD_NUM_SLOTS(md)) {
> >                       target_slot -= MD_NUM_SLOTS(md);
> >                       continue;
Kefeng Wang May 6, 2022, 1:06 a.m. UTC | #3
On 2022/5/5 21:43, Kefeng Wang wrote:
>
> On 2022/5/3 23:21, Ard Biesheuvel wrote:
>> If the system exposes memory regions with the EFI_MORE_RELIABLE
>> attribute, it is implied that it is intended to be used for allocations
>> that are relatively important, such as the kernel's static image.
>>
>> Since efi_random_alloc() is mostly (only) used for allocating space for
>> the kernel image, let's update it to take this into account, and
>> disregard all memory without the EFI_MORE_RELIABLE attribute if there is
>> sufficient memory available that does have this attribute.
>>
>> Note that this change only affects booting with randomization enabled.
>> In other cases, the EFI stub runs the kernel image in place unless its
>> placement is unsuitable for some reason (i.e., misaligned, or its BSS
>> overlaps with another allocation), and it is left to the bootloader to
>> ensure that the kernel was loaded into EFI_MORE_RELIABLE memory if this
>> is desired.
>>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   drivers/firmware/efi/libstub/randomalloc.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/libstub/randomalloc.c 
>> b/drivers/firmware/efi/libstub/randomalloc.c
>> index 724155b9e10d..07a762910312 100644
>> --- a/drivers/firmware/efi/libstub/randomalloc.c
>> +++ b/drivers/firmware/efi/libstub/randomalloc.c
>> @@ -56,6 +56,7 @@ efi_status_t efi_random_alloc(unsigned long size,
>>                     unsigned long random_seed)
>>   {
>>       unsigned long map_size, desc_size, total_slots = 0, target_slot;
>> +    unsigned long total_mirrored_slots = 0;
>>       unsigned long buff_size;
>>       efi_status_t status;
>>       efi_memory_desc_t *memory_map;
>> @@ -86,8 +87,14 @@ efi_status_t efi_random_alloc(unsigned long size,
>>           slots = get_entry_num_slots(md, size, ilog2(align));
>>           MD_NUM_SLOTS(md) = slots;
>>           total_slots += slots;
>> +        if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>> +            total_mirrored_slots += slots;
>>       }
>>   +    /* only consider mirrored slots for randomization if any exist */
>> +    if (total_mirrored_slots > 0)
>> +        total_slots = total_mirrored_slots;
>> +
>
> The kernel will check 4G lower limit to enable kernelcore=mirror feature.

Please ignore this, the kernel only check whether the memory below 4G are

all mirrored memory or not.

>
> Do we need some fallback mechanism in case of small mirror slots which
>
> leads to fail allocation for Image?

>
>
>>       /* find a random number between 0 and total_slots */
>>       target_slot = (total_slots * (u64)(random_seed & U32_MAX)) >> 32;
>>   @@ -107,6 +114,10 @@ efi_status_t efi_random_alloc(unsigned long size,
>>           efi_physical_addr_t target;
>>           unsigned long pages;
>>   +        if (total_mirrored_slots > 0 &&
>> +            !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
>> +            continue;
>> +
>>           if (target_slot >= MD_NUM_SLOTS(md)) {
>>               target_slot -= MD_NUM_SLOTS(md);
>>               continue;
Kefeng Wang May 6, 2022, 1:42 a.m. UTC | #4
On 2022/5/6 0:12, Ard Biesheuvel wrote:
> On Thu, 5 May 2022 at 15:43, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>> On 2022/5/3 23:21, Ard Biesheuvel wrote:
>>> If the system exposes memory regions with the EFI_MORE_RELIABLE
>>> attribute, it is implied that it is intended to be used for allocations
>>> that are relatively important, such as the kernel's static image.
>>>
>>> Since efi_random_alloc() is mostly (only) used for allocating space for
>>> the kernel image, let's update it to take this into account, and
>>> disregard all memory without the EFI_MORE_RELIABLE attribute if there is
>>> sufficient memory available that does have this attribute.
>>>
>>> Note that this change only affects booting with randomization enabled.
>>> In other cases, the EFI stub runs the kernel image in place unless its
>>> placement is unsuitable for some reason (i.e., misaligned, or its BSS
>>> overlaps with another allocation), and it is left to the bootloader to
>>> ensure that the kernel was loaded into EFI_MORE_RELIABLE memory if this
>>> is desired.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    drivers/firmware/efi/libstub/randomalloc.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
>>> index 724155b9e10d..07a762910312 100644
>>> --- a/drivers/firmware/efi/libstub/randomalloc.c
>>> +++ b/drivers/firmware/efi/libstub/randomalloc.c
>>> @@ -56,6 +56,7 @@ efi_status_t efi_random_alloc(unsigned long size,
>>>                              unsigned long random_seed)
>>>    {
>>>        unsigned long map_size, desc_size, total_slots = 0, target_slot;
>>> +     unsigned long total_mirrored_slots = 0;
>>>        unsigned long buff_size;
>>>        efi_status_t status;
>>>        efi_memory_desc_t *memory_map;
>>> @@ -86,8 +87,14 @@ efi_status_t efi_random_alloc(unsigned long size,
>>>                slots = get_entry_num_slots(md, size, ilog2(align));
>>>                MD_NUM_SLOTS(md) = slots;
>>>                total_slots += slots;
>>> +             if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>>> +                     total_mirrored_slots += slots;
>>>        }
>>>
>>> +     /* only consider mirrored slots for randomization if any exist */
>>> +     if (total_mirrored_slots > 0)
>>> +             total_slots = total_mirrored_slots;
>>> +
>> The kernel will check 4G lower limit to enable kernelcore=mirror feature.
>>
> Why? I mean, why is 4G a magic number also on arm64?
Please ignore this, replied in the previous email.
>
>> Do we need some fallback mechanism in case of small mirror slots which
>>
>> leads to fail allocation for Image?
>>
> This code only counts slots that are large enough to hold the Image so
> this can never happen. If total_mirrored_slots > 0, there is at least
> one possible placement of the kernel where it falls entirely inside a
> EFI_MORE_RELIABLE region.

I see,  slots = get_entry_num_slots(md, *size*, ilog2(align));

Thanks.

Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

>
>>>        /* find a random number between 0 and total_slots */
>>>        target_slot = (total_slots * (u64)(random_seed & U32_MAX)) >> 32;
>>>
>>> @@ -107,6 +114,10 @@ efi_status_t efi_random_alloc(unsigned long size,
>>>                efi_physical_addr_t target;
>>>                unsigned long pages;
>>>
>>> +             if (total_mirrored_slots > 0 &&
>>> +                 !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
>>> +                     continue;
>>> +
>>>                if (target_slot >= MD_NUM_SLOTS(md)) {
>>>                        target_slot -= MD_NUM_SLOTS(md);
>>>                        continue;
> .
Ard Biesheuvel May 6, 2022, 6:19 a.m. UTC | #5
On Fri, 6 May 2022 at 03:43, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
> On 2022/5/6 0:12, Ard Biesheuvel wrote:
> > On Thu, 5 May 2022 at 15:43, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >>
> >> On 2022/5/3 23:21, Ard Biesheuvel wrote:
> >>> If the system exposes memory regions with the EFI_MORE_RELIABLE
> >>> attribute, it is implied that it is intended to be used for allocations
> >>> that are relatively important, such as the kernel's static image.
> >>>
> >>> Since efi_random_alloc() is mostly (only) used for allocating space for
> >>> the kernel image, let's update it to take this into account, and
> >>> disregard all memory without the EFI_MORE_RELIABLE attribute if there is
> >>> sufficient memory available that does have this attribute.
> >>>
> >>> Note that this change only affects booting with randomization enabled.
> >>> In other cases, the EFI stub runs the kernel image in place unless its
> >>> placement is unsuitable for some reason (i.e., misaligned, or its BSS
> >>> overlaps with another allocation), and it is left to the bootloader to
> >>> ensure that the kernel was loaded into EFI_MORE_RELIABLE memory if this
> >>> is desired.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>    drivers/firmware/efi/libstub/randomalloc.c | 11 +++++++++++
> >>>    1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
> >>> index 724155b9e10d..07a762910312 100644
> >>> --- a/drivers/firmware/efi/libstub/randomalloc.c
> >>> +++ b/drivers/firmware/efi/libstub/randomalloc.c
> >>> @@ -56,6 +56,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> >>>                              unsigned long random_seed)
> >>>    {
> >>>        unsigned long map_size, desc_size, total_slots = 0, target_slot;
> >>> +     unsigned long total_mirrored_slots = 0;
> >>>        unsigned long buff_size;
> >>>        efi_status_t status;
> >>>        efi_memory_desc_t *memory_map;
> >>> @@ -86,8 +87,14 @@ efi_status_t efi_random_alloc(unsigned long size,
> >>>                slots = get_entry_num_slots(md, size, ilog2(align));
> >>>                MD_NUM_SLOTS(md) = slots;
> >>>                total_slots += slots;
> >>> +             if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> >>> +                     total_mirrored_slots += slots;
> >>>        }
> >>>
> >>> +     /* only consider mirrored slots for randomization if any exist */
> >>> +     if (total_mirrored_slots > 0)
> >>> +             total_slots = total_mirrored_slots;
> >>> +
> >> The kernel will check 4G lower limit to enable kernelcore=mirror feature.
> >>
> > Why? I mean, why is 4G a magic number also on arm64?
> Please ignore this, replied in the previous email.
> >
> >> Do we need some fallback mechanism in case of small mirror slots which
> >>
> >> leads to fail allocation for Image?
> >>
> > This code only counts slots that are large enough to hold the Image so
> > this can never happen. If total_mirrored_slots > 0, there is at least
> > one possible placement of the kernel where it falls entirely inside a
> > EFI_MORE_RELIABLE region.
>
> I see,  slots = get_entry_num_slots(md, *size*, ilog2(align));
>
> Thanks.
>
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>

Thank you. I have queued this up now.
diff mbox series

Patch

diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
index 724155b9e10d..07a762910312 100644
--- a/drivers/firmware/efi/libstub/randomalloc.c
+++ b/drivers/firmware/efi/libstub/randomalloc.c
@@ -56,6 +56,7 @@  efi_status_t efi_random_alloc(unsigned long size,
 			      unsigned long random_seed)
 {
 	unsigned long map_size, desc_size, total_slots = 0, target_slot;
+	unsigned long total_mirrored_slots = 0;
 	unsigned long buff_size;
 	efi_status_t status;
 	efi_memory_desc_t *memory_map;
@@ -86,8 +87,14 @@  efi_status_t efi_random_alloc(unsigned long size,
 		slots = get_entry_num_slots(md, size, ilog2(align));
 		MD_NUM_SLOTS(md) = slots;
 		total_slots += slots;
+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
+			total_mirrored_slots += slots;
 	}
 
+	/* only consider mirrored slots for randomization if any exist */
+	if (total_mirrored_slots > 0)
+		total_slots = total_mirrored_slots;
+
 	/* find a random number between 0 and total_slots */
 	target_slot = (total_slots * (u64)(random_seed & U32_MAX)) >> 32;
 
@@ -107,6 +114,10 @@  efi_status_t efi_random_alloc(unsigned long size,
 		efi_physical_addr_t target;
 		unsigned long pages;
 
+		if (total_mirrored_slots > 0 &&
+		    !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
+			continue;
+
 		if (target_slot >= MD_NUM_SLOTS(md)) {
 			target_slot -= MD_NUM_SLOTS(md);
 			continue;