2021-02-24 18:48:50

by Kairui Song

[permalink] [raw]
Subject: [PATCH] efi: memmap insertion should adjust the vaddr as well

Currently when efi_memmap_insert is called, only the
physical memory addresses are re-calculated. The virt
addresses of the split entries are untouched.

If any later operation depends on the virt_addaress info, things
will go wrong. One case it may fail is kexec on x86, after kexec,
efi is already in virtual mode, kernel simply do fixed mapping
reuse the recorded virt address. If the virt address is incorrect,
the mapping will be invalid.

Update the virt_addaress as well when inserting a memmap entry to
fix this potential issue.

Signed-off-by: Kairui Song <[email protected]>
---
drivers/firmware/efi/memmap.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 2ff1883dc788..de5c545b2074 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -292,7 +292,7 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
{
u64 m_start, m_end, m_attr;
efi_memory_desc_t *md;
- u64 start, end;
+ u64 start, end, virt_offset;
void *old, *new;

/* modifying range */
@@ -321,6 +321,11 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
start = md->phys_addr;
end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;

+ if (md->virt_addr)
+ virt_offset = md->virt_addr - md->phys_addr;
+ else
+ virt_offset = -1;
+
if (m_start <= start && end <= m_end)
md->attribute |= m_attr;

@@ -337,6 +342,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
md->phys_addr = m_end + 1;
md->num_pages = (end - md->phys_addr + 1) >>
EFI_PAGE_SHIFT;
+ if (virt_offset != -1)
+ md->virt_addr = md->phys_addr + virt_offset;
}

if ((start < m_start && m_start < end) && m_end < end) {
@@ -351,6 +358,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
md->phys_addr = m_start;
md->num_pages = (m_end - m_start + 1) >>
EFI_PAGE_SHIFT;
+ if (virt_offset != -1)
+ md->virt_addr = md->phys_addr + virt_offset;
/* last part */
new += old_memmap->desc_size;
memcpy(new, old, old_memmap->desc_size);
@@ -358,6 +367,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
md->phys_addr = m_end + 1;
md->num_pages = (end - m_end) >>
EFI_PAGE_SHIFT;
+ if (virt_offset != -1)
+ md->virt_addr = md->phys_addr + virt_offset;
}

if ((start < m_start && m_start < end) &&
@@ -373,6 +384,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
md->num_pages = (end - md->phys_addr + 1) >>
EFI_PAGE_SHIFT;
md->attribute |= m_attr;
+ if (virt_offset != -1)
+ md->virt_addr = md->phys_addr + virt_offset;
}
}
}
--
2.29.2


2021-03-08 06:54:31

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] efi: memmap insertion should adjust the vaddr as well

On Wed, 24 Feb 2021 at 19:43, Kairui Song <[email protected]> wrote:
>
> Currently when efi_memmap_insert is called, only the
> physical memory addresses are re-calculated. The virt
> addresses of the split entries are untouched.
>
> If any later operation depends on the virt_addaress info, things
> will go wrong. One case it may fail is kexec on x86, after kexec,
> efi is already in virtual mode, kernel simply do fixed mapping
> reuse the recorded virt address. If the virt address is incorrect,
> the mapping will be invalid.
>
> Update the virt_addaress as well when inserting a memmap entry to
> fix this potential issue.
>
> Signed-off-by: Kairui Song <[email protected]>
> ---
> drivers/firmware/efi/memmap.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 2ff1883dc788..de5c545b2074 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -292,7 +292,7 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> {
> u64 m_start, m_end, m_attr;
> efi_memory_desc_t *md;
> - u64 start, end;
> + u64 start, end, virt_offset;
> void *old, *new;
>
> /* modifying range */
> @@ -321,6 +321,11 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> start = md->phys_addr;
> end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
>
> + if (md->virt_addr)
> + virt_offset = md->virt_addr - md->phys_addr;
> + else
> + virt_offset = -1;
> +

Should we take EFI_MEMORY_RUNTIME into account here? I don't think we
should expect to see non-zero virtual addresses if that attribute is
missing.


> if (m_start <= start && end <= m_end)
> md->attribute |= m_attr;
>
> @@ -337,6 +342,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> md->phys_addr = m_end + 1;
> md->num_pages = (end - md->phys_addr + 1) >>
> EFI_PAGE_SHIFT;
> + if (virt_offset != -1)
> + md->virt_addr = md->phys_addr + virt_offset;
> }
>
> if ((start < m_start && m_start < end) && m_end < end) {
> @@ -351,6 +358,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> md->phys_addr = m_start;
> md->num_pages = (m_end - m_start + 1) >>
> EFI_PAGE_SHIFT;
> + if (virt_offset != -1)
> + md->virt_addr = md->phys_addr + virt_offset;
> /* last part */
> new += old_memmap->desc_size;
> memcpy(new, old, old_memmap->desc_size);
> @@ -358,6 +367,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> md->phys_addr = m_end + 1;
> md->num_pages = (end - m_end) >>
> EFI_PAGE_SHIFT;
> + if (virt_offset != -1)
> + md->virt_addr = md->phys_addr + virt_offset;
> }
>
> if ((start < m_start && m_start < end) &&
> @@ -373,6 +384,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> md->num_pages = (end - md->phys_addr + 1) >>
> EFI_PAGE_SHIFT;
> md->attribute |= m_attr;
> + if (virt_offset != -1)
> + md->virt_addr = md->phys_addr + virt_offset;
> }
> }
> }
> --
> 2.29.2
>