| Message ID | 20221003112625.972646-5-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efi/x86: Avoid corrupted config tables under Xen | expand |
On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote: > The ESRT code currently contains some sanity checks on the memory > descriptor it obtains, but these can only trigger when the descriptor is > invalid (if at all). > > So let's drop these checks, and instead, disregard descriptors entirely > if the start address is misaligned, or the number of pages reaches > beyond the end of the address space. Note that the memory map as a whole > could still be inconsistent, i.e., multiple entries might cover the same > area, or the address could be outside of the addressable VA space, but > validating that goes beyond the scope of these helpers. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/firmware/efi/efi.c | 13 +++++++------ > drivers/firmware/efi/esrt.c | 18 +----------------- > 2 files changed, 8 insertions(+), 23 deletions(-) > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 11857af72859..55bd3f4aab28 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) > efi_memory_desc_t *md; > > if (!efi_enabled(EFI_MEMMAP)) { > - pr_err_once("EFI_MEMMAP is not enabled.\n"); > + pr_warn_once("EFI_MEMMAP is not enabled.\n"); > return -EINVAL; > } > > - if (!out_md) { > - pr_err_once("out_md is null.\n"); > - return -EINVAL; > - } > - Nit: this seems unrelated. > for_each_efi_memory_desc(md) { > u64 size; > u64 end; > > + /* skip bogus entries */ > + if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) || > + (md->phys_addr > 0 && > + (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT))) > + continue; Should this also check if md->num_pages is 0? Also, should this check be part of for_each_efi_memory_desc()? > + > size = md->num_pages << EFI_PAGE_SHIFT; > end = md->phys_addr + size; > if (phys_addr >= md->phys_addr && phys_addr < end) { > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c > index 2a2f52b017e7..8f86f2b0734b 100644 > --- a/drivers/firmware/efi/esrt.c > +++ b/drivers/firmware/efi/esrt.c > @@ -247,9 +247,6 @@ void __init efi_esrt_init(void) > int rc; > phys_addr_t end; > > - if (!efi_enabled(EFI_MEMMAP)) > - return; > - > pr_debug("esrt-init: loading.\n"); > if (!esrt_table_exists()) > return; > @@ -263,21 +260,8 @@ void __init efi_esrt_init(void) > return; > } > > - max = efi_mem_desc_end(&md); > - if (max < efi.esrt) { > - pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n", > - (void *)efi.esrt, (void *)max); > - return; > - } > - > + max = efi_mem_desc_end(&md) - efi.esrt; > size = sizeof(*esrt); > - max -= efi.esrt; > - > - if (max < size) { > - pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n", > - size, max); > - return; > - } This can still happen if the ESRT pointer is very very close to the end of a memory map entry, unless there is another check that handles such cases.
On Mon, 3 Oct 2022 at 17:18, Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > > On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote: > > The ESRT code currently contains some sanity checks on the memory > > descriptor it obtains, but these can only trigger when the descriptor is > > invalid (if at all). > > > > So let's drop these checks, and instead, disregard descriptors entirely > > if the start address is misaligned, or the number of pages reaches > > beyond the end of the address space. Note that the memory map as a whole > > could still be inconsistent, i.e., multiple entries might cover the same > > area, or the address could be outside of the addressable VA space, but > > validating that goes beyond the scope of these helpers. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > drivers/firmware/efi/efi.c | 13 +++++++------ > > drivers/firmware/efi/esrt.c | 18 +----------------- > > 2 files changed, 8 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > > index 11857af72859..55bd3f4aab28 100644 > > --- a/drivers/firmware/efi/efi.c > > +++ b/drivers/firmware/efi/efi.c > > @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) > > efi_memory_desc_t *md; > > > > if (!efi_enabled(EFI_MEMMAP)) { > > - pr_err_once("EFI_MEMMAP is not enabled.\n"); > > + pr_warn_once("EFI_MEMMAP is not enabled.\n"); > > return -EINVAL; > > } > > > > - if (!out_md) { > > - pr_err_once("out_md is null.\n"); > > - return -EINVAL; > > - } > > - > > Nit: this seems unrelated. > > > for_each_efi_memory_desc(md) { > > u64 size; > > u64 end; > > > > + /* skip bogus entries */ > > + if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) || > > + (md->phys_addr > 0 && > > + (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT))) > > + continue; > > Should this also check if md->num_pages is 0? Yes, probably. > Also, should this check > be part of for_each_efi_memory_desc()? > No, I don't think so. The for_each_xxx() helpers we have throughout the kernel usually don't incorporate such checks, and I'd prefer to adhere to the principle of least surprise here. > > + > > size = md->num_pages << EFI_PAGE_SHIFT; > > end = md->phys_addr + size; > > if (phys_addr >= md->phys_addr && phys_addr < end) { > > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c > > index 2a2f52b017e7..8f86f2b0734b 100644 > > --- a/drivers/firmware/efi/esrt.c > > +++ b/drivers/firmware/efi/esrt.c > > @@ -247,9 +247,6 @@ void __init efi_esrt_init(void) > > int rc; > > phys_addr_t end; > > > > - if (!efi_enabled(EFI_MEMMAP)) > > - return; > > - > > pr_debug("esrt-init: loading.\n"); > > if (!esrt_table_exists()) > > return; > > @@ -263,21 +260,8 @@ void __init efi_esrt_init(void) > > return; > > } > > > > - max = efi_mem_desc_end(&md); > > - if (max < efi.esrt) { > > - pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n", > > - (void *)efi.esrt, (void *)max); > > - return; > > - } > > - > > + max = efi_mem_desc_end(&md) - efi.esrt; > > size = sizeof(*esrt); > > - max -= efi.esrt; > > - > > - if (max < size) { > > - pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n", > > - size, max); > > - return; > > - } > > This can still happen if the ESRT pointer is very very close to the end > of a memory map entry, unless there is another check that handles > such cases. You're right - I missed that.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 11857af72859..55bd3f4aab28 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) efi_memory_desc_t *md; if (!efi_enabled(EFI_MEMMAP)) { - pr_err_once("EFI_MEMMAP is not enabled.\n"); + pr_warn_once("EFI_MEMMAP is not enabled.\n"); return -EINVAL; } - if (!out_md) { - pr_err_once("out_md is null.\n"); - return -EINVAL; - } - for_each_efi_memory_desc(md) { u64 size; u64 end; + /* skip bogus entries */ + if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) || + (md->phys_addr > 0 && + (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT))) + continue; + size = md->num_pages << EFI_PAGE_SHIFT; end = md->phys_addr + size; if (phys_addr >= md->phys_addr && phys_addr < end) { diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c index 2a2f52b017e7..8f86f2b0734b 100644 --- a/drivers/firmware/efi/esrt.c +++ b/drivers/firmware/efi/esrt.c @@ -247,9 +247,6 @@ void __init efi_esrt_init(void) int rc; phys_addr_t end; - if (!efi_enabled(EFI_MEMMAP)) - return; - pr_debug("esrt-init: loading.\n"); if (!esrt_table_exists()) return; @@ -263,21 +260,8 @@ void __init efi_esrt_init(void) return; } - max = efi_mem_desc_end(&md); - if (max < efi.esrt) { - pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n", - (void *)efi.esrt, (void *)max); - return; - } - + max = efi_mem_desc_end(&md) - efi.esrt; size = sizeof(*esrt); - max -= efi.esrt; - - if (max < size) { - pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n", - size, max); - return; - } va = early_memremap(efi.esrt, size); if (!va) {
The ESRT code currently contains some sanity checks on the memory descriptor it obtains, but these can only trigger when the descriptor is invalid (if at all). So let's drop these checks, and instead, disregard descriptors entirely if the start address is misaligned, or the number of pages reaches beyond the end of the address space. Note that the memory map as a whole could still be inconsistent, i.e., multiple entries might cover the same area, or the address could be outside of the addressable VA space, but validating that goes beyond the scope of these helpers. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/firmware/efi/efi.c | 13 +++++++------ drivers/firmware/efi/esrt.c | 18 +----------------- 2 files changed, 8 insertions(+), 23 deletions(-)