xen/common/efi/boot.c | 11 +++++------ xen/common/efi/efi.h | 14 ++++++++++++++ xen/common/efi/runtime.c | 13 ++++++------- xen/include/xen/types.h | 1 + 4 files changed, 26 insertions(+), 13 deletions(-)
It turns out that these can be invalid in various ways. Based on code
Ard Biesheuvel contributed for Linux.
Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
xen/common/efi/boot.c | 11 +++++------
xen/common/efi/efi.h | 14 ++++++++++++++
xen/common/efi/runtime.c | 13 ++++++-------
xen/include/xen/types.h | 1 +
4 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b3de1011ee58a67a82a94da050eb1343f4e37faa..dd0376fdf930c2fee35e79a2f2821361c5e15d33 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -591,15 +591,14 @@ static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
{
- size_t available_len, len;
+ UINT64 available_len, len = efi_memory_descriptor_len(desc);
const UINTN physical_start = desc->PhysicalStart;
const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
- len = desc->NumberOfPages << EFI_PAGE_SHIFT;
if ( esrt == EFI_INVALID_TABLE_ADDR )
- return 0;
+ return 0; /* invalid ESRT */
if ( physical_start > esrt || esrt - physical_start >= len )
- return 0;
+ return 0; /* ESRT not in this memory region */
/*
* The specification requires EfiBootServicesData, but also accept
* EfiRuntimeServicesData (for compatibility with buggy firmware)
@@ -609,10 +608,10 @@ static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
if ( (desc->Type != EfiRuntimeServicesData) &&
(desc->Type != EfiBootServicesData) &&
(desc->Type != EfiACPIReclaimMemory) )
- return 0;
+ return 0; /* memory region cannot contain ESRT */
available_len = len - (esrt - physical_start);
if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
- return 0;
+ return 0; /* ESRT header does not fit in memory region */
available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
if ( (esrt_ptr->FwResourceVersion !=
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c9aa65d506b14de69c90b6538c934747fcf0fb80..f4875138415c23eac42616131a738b7f8e33e56b 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
const void *pe_find_section(const void *image_base, const size_t image_size,
const CHAR16 *section_name, UINTN *size_out);
+
+static inline UINT64
+efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
+{
+ uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;
+
+ if ( desc->PhysicalStart & (EFI_PAGE_SIZE - 1) )
+ return 0; /* misaligned start address */
+
+ if ( desc->NumberOfPages > (remaining_space >> EFI_PAGE_SHIFT) )
+ return 0; /* too big */
+
+ return desc->NumberOfPages << EFI_PAGE_SHIFT;
+}
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 13b0975866e3a80c46a7e37788012a716a455b6a..b99de230a5464c46b0b6c19b073685b4e0298343 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
{
EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
- u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+ uint64_t size, len = efi_memory_descriptor_len(desc);
if ( info->mem.addr >= desc->PhysicalStart &&
- info->mem.addr < desc->PhysicalStart + len )
+ info->mem.addr - desc->PhysicalStart < len )
{
info->mem.type = desc->Type;
info->mem.attr = desc->Attribute;
- if ( info->mem.addr + info->mem.size < info->mem.addr ||
- info->mem.addr + info->mem.size >
- desc->PhysicalStart + len )
- info->mem.size = desc->PhysicalStart + len -
- info->mem.addr;
+ size = desc->PhysicalStart + len - info->mem.addr;
+ if ( info->mem.size > size )
+ info->mem.size = size;
+
return 0;
}
}
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 03f0fe612ed96118614505c39d0e33b946288d6c..255f5a3c91c6be3e8ba4584902563c11bee7f98d 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -25,6 +25,7 @@
#define UINT8_MAX (255)
#define UINT16_MAX (65535)
#define UINT32_MAX (4294967295U)
+#define UINT64_MAX (0xFFFFFFFFFFFFFFFFULL)
#define INT_MAX ((int)(~0U>>1))
#define INT_MIN (-INT_MAX - 1)
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things LabOn 07.12.2022 00:57, Demi Marie Obenour wrote:
> It turns out that these can be invalid in various ways. Based on code
> Ard Biesheuvel contributed for Linux.
>
> Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
This comes with the risk of breaking something that was previously working,
despite a descriptor being bogus. This _may_ be deemed acceptable, but it
needs calling out and reasoning about in the description. Even more so that
elsewhere we're aiming at relaxing things (by default or via command line
overrides) to remain compatible with all kinds of flawed implementations.
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
>
> const void *pe_find_section(const void *image_base, const size_t image_size,
> const CHAR16 *section_name, UINTN *size_out);
> +
> +static inline UINT64
> +efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
> +{
> + uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;
This wants to derive from PADDR_BITS (or even paddr_bits) rather than
using UINT64_MAX.
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> {
> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> - u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> + uint64_t size, len = efi_memory_descriptor_len(desc);
>
> if ( info->mem.addr >= desc->PhysicalStart &&
> - info->mem.addr < desc->PhysicalStart + len )
> + info->mem.addr - desc->PhysicalStart < len )
With what efi_memory_descriptor_len() does I don't see why this expression
would need transformation - there's no overflow possible anymore.
> {
> info->mem.type = desc->Type;
> info->mem.attr = desc->Attribute;
> - if ( info->mem.addr + info->mem.size < info->mem.addr ||
This overflow check is not superseded by the use of
efi_memory_descriptor_len(), so if you think it is not (or no longer)
needed, you will need to justify that in the description.
> - info->mem.addr + info->mem.size >
> - desc->PhysicalStart + len )
> - info->mem.size = desc->PhysicalStart + len -
> - info->mem.addr;
> + size = desc->PhysicalStart + len - info->mem.addr;
> + if ( info->mem.size > size )
> + info->mem.size = size;
> +
> return 0;
> }
Is there any functional change in here that I'm overlooking, or are you
merely converting this code to meet your personal taste? In the latter
case I'd prefer if you left the code untouched, with the 2nd best option
being to split it to a separate (style only) patch, and the 3rd option
being to at least mention this as a no-op (style only) transformation in
the description.
Jan
On Wed, Dec 07, 2022 at 11:04:05AM +0100, Jan Beulich wrote:
> On 07.12.2022 00:57, Demi Marie Obenour wrote:
> > It turns out that these can be invalid in various ways. Based on code
> > Ard Biesheuvel contributed for Linux.
> >
> > Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> This comes with the risk of breaking something that was previously working,
> despite a descriptor being bogus. This _may_ be deemed acceptable, but it
> needs calling out and reasoning about in the description. Even more so that
> elsewhere we're aiming at relaxing things (by default or via command line
> overrides) to remain compatible with all kinds of flawed implementations.
I decided to match Ard’s kernel patch, except for ignoring (as opposed
to using) descriptors that cover the entire 64-bit address space (and
thus have a length in bytes that overflows uint64_t).
What do you propose Xen do instead? A broken memory map is a rather
serious problem; it means that the actual physical address space is
unknown. For Linux I am considering tainting the kernel (with
TAINT_FIRMWARE_WORKAROUND) if it detects an invalid memory descriptor.
> > --- a/xen/common/efi/efi.h
> > +++ b/xen/common/efi/efi.h
> > @@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
> >
> > const void *pe_find_section(const void *image_base, const size_t image_size,
> > const CHAR16 *section_name, UINTN *size_out);
> > +
> > +static inline UINT64
> > +efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
> > +{
> > + uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;
>
> This wants to derive from PADDR_BITS (or even paddr_bits) rather than
> using UINT64_MAX.
paddr_bits is not available yet, but I can use PADDR_BITS. That does
require an explicit overflow check, but that is fine.
> > --- a/xen/common/efi/runtime.c
> > +++ b/xen/common/efi/runtime.c
> > @@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> > for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> > {
> > EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> > - u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> > + uint64_t size, len = efi_memory_descriptor_len(desc);
> >
> > if ( info->mem.addr >= desc->PhysicalStart &&
> > - info->mem.addr < desc->PhysicalStart + len )
> > + info->mem.addr - desc->PhysicalStart < len )
>
> With what efi_memory_descriptor_len() does I don't see why this expression
> would need transformation - there's no overflow possible anymore.
You are correct, but the new version is more idiomatic, IMO.
> > {
> > info->mem.type = desc->Type;
> > info->mem.attr = desc->Attribute;
> > - if ( info->mem.addr + info->mem.size < info->mem.addr ||
>
> This overflow check is not superseded by the use of
> efi_memory_descriptor_len(), so if you think it is not (or no longer)
> needed, you will need to justify that in the description.
The justification is that info->mem.size is no longer used except in
comparison and assignment, so the overflow check is now redundant.
> > - info->mem.addr + info->mem.size >
> > - desc->PhysicalStart + len )
> > - info->mem.size = desc->PhysicalStart + len -
> > - info->mem.addr;
> > + size = desc->PhysicalStart + len - info->mem.addr;
> > + if ( info->mem.size > size )
> > + info->mem.size = size;
> > +
> > return 0;
> > }
>
> Is there any functional change in here that I'm overlooking, or are you
> merely converting this code to meet your personal taste? In the latter
> case I'd prefer if you left the code untouched, with the 2nd best option
> being to split it to a separate (style only) patch, and the 3rd option
> being to at least mention this as a no-op (style only) transformation in
> the description.
There should be no functional change here, but IMO the new version is
much easier to read: first compute the actual size, and if the provided
size is larger, clamp it.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 07.12.2022 23:23, Demi Marie Obenour wrote:
> On Wed, Dec 07, 2022 at 11:04:05AM +0100, Jan Beulich wrote:
>> On 07.12.2022 00:57, Demi Marie Obenour wrote:
>>> It turns out that these can be invalid in various ways. Based on code
>>> Ard Biesheuvel contributed for Linux.
>>>
>>> Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
>>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>
>> This comes with the risk of breaking something that was previously working,
>> despite a descriptor being bogus. This _may_ be deemed acceptable, but it
>> needs calling out and reasoning about in the description. Even more so that
>> elsewhere we're aiming at relaxing things (by default or via command line
>> overrides) to remain compatible with all kinds of flawed implementations.
>
> I decided to match Ard’s kernel patch, except for ignoring (as opposed
> to using) descriptors that cover the entire 64-bit address space (and
> thus have a length in bytes that overflows uint64_t).
>
> What do you propose Xen do instead? A broken memory map is a rather
> serious problem; it means that the actual physical address space is
> unknown. For Linux I am considering tainting the kernel (with
> TAINT_FIRMWARE_WORKAROUND) if it detects an invalid memory descriptor.
Much here depends on what brokenness we find in practice. I've been
flamed more than once for insisting on strict spec compliance. I'd
be fine with you making things more strict here, but then - as said -
the risks need to be called out in the description, and once you've
done so you'll likely realize yourself that hence there then needs
to be a way out for systems where the new checking turns out too
strict.
Tainting the hypervisor in the event of finding an issue is certainly
an option.
>>> --- a/xen/common/efi/efi.h
>>> +++ b/xen/common/efi/efi.h
>>> @@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
>>>
>>> const void *pe_find_section(const void *image_base, const size_t image_size,
>>> const CHAR16 *section_name, UINTN *size_out);
>>> +
>>> +static inline UINT64
>>> +efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
>>> +{
>>> + uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;
>>
>> This wants to derive from PADDR_BITS (or even paddr_bits) rather than
>> using UINT64_MAX.
>
> paddr_bits is not available yet, but I can use PADDR_BITS. That does
> require an explicit overflow check, but that is fine.
paddr_bits may or may not be available yet; it certainly is ...
>>> --- a/xen/common/efi/runtime.c
>>> +++ b/xen/common/efi/runtime.c
>>> @@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>> for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>>> {
>>> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>>> - u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
>>> + uint64_t size, len = efi_memory_descriptor_len(desc);
... for this use.
>>> if ( info->mem.addr >= desc->PhysicalStart &&
>>> - info->mem.addr < desc->PhysicalStart + len )
>>> + info->mem.addr - desc->PhysicalStart < len )
>>
>> With what efi_memory_descriptor_len() does I don't see why this expression
>> would need transformation - there's no overflow possible anymore.
>
> You are correct, but the new version is more idiomatic, IMO.
>
>>> {
>>> info->mem.type = desc->Type;
>>> info->mem.attr = desc->Attribute;
>>> - if ( info->mem.addr + info->mem.size < info->mem.addr ||
>>
>> This overflow check is not superseded by the use of
>> efi_memory_descriptor_len(), so if you think it is not (or no longer)
>> needed, you will need to justify that in the description.
>
> The justification is that info->mem.size is no longer used except in
> comparison and assignment, so the overflow check is now redundant.
I don't see what "no longer" refers to - nothing changes in this regard
before and after your patch, afaics.
>>> - info->mem.addr + info->mem.size >
>>> - desc->PhysicalStart + len )
>>> - info->mem.size = desc->PhysicalStart + len -
>>> - info->mem.addr;
>>> + size = desc->PhysicalStart + len - info->mem.addr;
>>> + if ( info->mem.size > size )
>>> + info->mem.size = size;
>>> +
>>> return 0;
>>> }
>>
>> Is there any functional change in here that I'm overlooking, or are you
>> merely converting this code to meet your personal taste? In the latter
>> case I'd prefer if you left the code untouched, with the 2nd best option
>> being to split it to a separate (style only) patch, and the 3rd option
>> being to at least mention this as a no-op (style only) transformation in
>> the description.
>
> There should be no functional change here, but IMO the new version is
> much easier to read: first compute the actual size, and if the provided
> size is larger, clamp it.
That's a matter of taste, as already indicated. My taste, for example, is
that you're introducing a new local variable for no good reason.
Jan
On Thu, Dec 08, 2022 at 09:02:57AM +0100, Jan Beulich wrote:
> On 07.12.2022 23:23, Demi Marie Obenour wrote:
> > On Wed, Dec 07, 2022 at 11:04:05AM +0100, Jan Beulich wrote:
> >> On 07.12.2022 00:57, Demi Marie Obenour wrote:
> >>> It turns out that these can be invalid in various ways. Based on code
> >>> Ard Biesheuvel contributed for Linux.
> >>>
> >>> Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
> >>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>
> >> This comes with the risk of breaking something that was previously working,
> >> despite a descriptor being bogus. This _may_ be deemed acceptable, but it
> >> needs calling out and reasoning about in the description. Even more so that
> >> elsewhere we're aiming at relaxing things (by default or via command line
> >> overrides) to remain compatible with all kinds of flawed implementations.
> >
> > I decided to match Ard’s kernel patch, except for ignoring (as opposed
> > to using) descriptors that cover the entire 64-bit address space (and
> > thus have a length in bytes that overflows uint64_t).
> >
> > What do you propose Xen do instead? A broken memory map is a rather
> > serious problem; it means that the actual physical address space is
> > unknown. For Linux I am considering tainting the kernel (with
> > TAINT_FIRMWARE_WORKAROUND) if it detects an invalid memory descriptor.
>
> Much here depends on what brokenness we find in practice. I've been
> flamed more than once for insisting on strict spec compliance. I'd
> be fine with you making things more strict here, but then - as said -
> the risks need to be called out in the description, and once you've
> done so you'll likely realize yourself that hence there then needs
> to be a way out for systems where the new checking turns out too
> strict.
The purpose of these checks is actually to work around broken firmware.
If firmware were bug-free, there would be no point in validating the
data it provides. That said, I am not sure if ignoring the broken
entries is the correct answer. Could Xen fall back to getting the
information from ACPI? Or could Xen somehow try to make sense of the
broken table? For instance, Xen could sort the entries by start address
and force the end of each entry to at or before the start of the next.
This patch has the advantage that it closely matches what Linux will be
doing. I trust Ard’s judgement on this better than my own.
> Tainting the hypervisor in the event of finding an issue is certainly
> an option.
I probably will not add such a mechanism, but if one exists I would be
happy to use it.
> >>> --- a/xen/common/efi/efi.h
> >>> +++ b/xen/common/efi/efi.h
> >>> @@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
> >>>
> >>> const void *pe_find_section(const void *image_base, const size_t image_size,
> >>> const CHAR16 *section_name, UINTN *size_out);
> >>> +
> >>> +static inline UINT64
> >>> +efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
> >>> +{
> >>> + uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;
> >>
> >> This wants to derive from PADDR_BITS (or even paddr_bits) rather than
> >> using UINT64_MAX.
> >
> > paddr_bits is not available yet, but I can use PADDR_BITS. That does
> > require an explicit overflow check, but that is fine.
>
> paddr_bits may or may not be available yet; it certainly is ...
>
> >>> --- a/xen/common/efi/runtime.c
> >>> +++ b/xen/common/efi/runtime.c
> >>> @@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> >>> for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> >>> {
> >>> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> >>> - u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> >>> + uint64_t size, len = efi_memory_descriptor_len(desc);
>
> ... for this use.
>
> >>> if ( info->mem.addr >= desc->PhysicalStart &&
> >>> - info->mem.addr < desc->PhysicalStart + len )
> >>> + info->mem.addr - desc->PhysicalStart < len )
> >>
> >> With what efi_memory_descriptor_len() does I don't see why this expression
> >> would need transformation - there's no overflow possible anymore.
> >
> > You are correct, but the new version is more idiomatic, IMO.
> >
> >>> {
> >>> info->mem.type = desc->Type;
> >>> info->mem.attr = desc->Attribute;
> >>> - if ( info->mem.addr + info->mem.size < info->mem.addr ||
> >>
> >> This overflow check is not superseded by the use of
> >> efi_memory_descriptor_len(), so if you think it is not (or no longer)
> >> needed, you will need to justify that in the description.
> >
> > The justification is that info->mem.size is no longer used except in
> > comparison and assignment, so the overflow check is now redundant.
>
> I don't see what "no longer" refers to - nothing changes in this regard
> before and after your patch, afaics.
The purpose of this check is to catch the case where info->mem.size +
info->mem.addr overflows. In the new code, there is no need to
explicitly check for this situation, because info->mem.size +
info->mem.addr is never actually computed. Instead, the size is
computed first, and if info->mem.size is larger than this, it is clamped
to the actual value.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 08.12.2022 10:36, Demi Marie Obenour wrote: > On Thu, Dec 08, 2022 at 09:02:57AM +0100, Jan Beulich wrote: >> Tainting the hypervisor in the event of finding an issue is certainly >> an option. > > I probably will not add such a mechanism, but if one exists I would be > happy to use it. See common/kernel.c:add_taint(). What you would need to introduce is a new TAINT_* constant (and its associated handling), unless we wanted to reuse (abuse) an existing one (yet none looks to even come just close). Jan
© 2016 - 2024 Red Hat, Inc.