[U-Boot] [PATCH 1/3] efi: fix issue for 32bit targets with ram_top at 4G
Avoid ram_end = 0 on 32bit targets with ram_end at 4G.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com --- example of issue in stm32mp1 board ev1: ram_start = c0000000 size = 40000000 ram_end = 100000000 ram_end &= ~EFI_PAGE_MASK => result is 0
lib/efi_loader/efi_memory.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 55622d2..81dc5fc 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -574,6 +574,10 @@ __weak void efi_add_known_memory(void)
/* Remove partial pages */ ram_end &= ~EFI_PAGE_MASK; + /* Fix for 32bit targets with ram_top at 4G */ + if (!ram_end) + ram_end = 0x100000000ULL; + ram_start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
if (ram_end <= ram_start) {
Check the value of block_dev before to use this pointer.
This patch solves problem for the command "load" when ubifs is previously mounted: in this case the function blk_get_device_part_str("ubi 0") don't return error but return block_dev = NULL and then data abort.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
To reproduce the issue, I have a boot script 'boot.scr.uimg' with a load command executed during ubi boot:
load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
I have a data abort for call stack: - do_load_wrapper for "ubi 0" -- efi_set_bootdev --- efi_dp_from_name
=> desc = 0 and data abort for access to 'desc->*'
I also proposed a protection for the same issue in ums command http://patchwork.ozlabs.org/project/uboot/list/?series=68096
lib/efi_loader/efi_device_path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 53b40c8..fd57be8 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (!is_net) { part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition, 1); - if (part < 0) + if (part < 0 || !desc) return EFI_INVALID_PARAMETER;
if (device)
On 4/10/19 11:02 AM, Patrick Delaunay wrote:
Check the value of block_dev before to use this pointer.
This patch solves problem for the command "load" when ubifs is previously mounted: in this case the function blk_get_device_part_str("ubi 0") don't return error but return block_dev = NULL and then data abort.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
To reproduce the issue, I have a boot script 'boot.scr.uimg' with a load command executed during ubi boot:
load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
I have a data abort for call stack:
- do_load_wrapper for "ubi 0"
-- efi_set_bootdev --- efi_dp_from_name
=> desc = 0 and data abort for access to 'desc->*'
Thanks for reporting and analyzing the problem
Where exactly is the NULL dereference occurring?
Igor reported a similar bug for a USB device in cmd: fs: fix data abort in load cmd https://lists.denx.de/pipermail/u-boot/2019-April/364484.htmll
I also proposed a protection for the same issue in ums command http://patchwork.ozlabs.org/project/uboot/list/?series=68096
lib/efi_loader/efi_device_path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 53b40c8..fd57be8 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (!is_net) { part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition, 1);
if (part < 0)
if (part < 0 || !desc)
part = 0, desc = NULL occurs for UBI if the UBI file system is mounted.
Returning an error here means in the end that we will not be able to install run GRUB from the UBI device because we cannot describe the boot device.
I think that UBI volumes should be handled like any other block device. This will avoid having separate program paths for UBI and not UBI.
Heiko and Kyungmin could you, please, explain why UBI currently is not providing a struct blk_desc * block descriptor and how this can be fixed.
Best regards
Heinrich
return EFI_INVALID_PARAMETER; if (device)
Hi Heinrich
From: Heinrich Schuchardt xypron.glpk@gmx.de
On 4/10/19 11:02 AM, Patrick Delaunay wrote:
Check the value of block_dev before to use this pointer.
This patch solves problem for the command "load" when ubifs is previously mounted: in this case the function blk_get_device_part_str("ubi 0") don't return error but return block_dev = NULL and then data abort.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
To reproduce the issue, I have a boot script 'boot.scr.uimg' with a load command executed during ubi boot:
load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
I have a data abort for call stack:
- do_load_wrapper for "ubi 0"
-- efi_set_bootdev --- efi_dp_from_name
=> desc = 0 and data abort for access to 'desc->*'
Thanks for reporting and analyzing the problem
Where exactly is the NULL dereference occurring?
I see the issue on v2018.11 and I adapt the patch for v2019.04 (as efi_dp_from_name as be created in v2019.01)
On v2018.11, the stack frame was
do_load_wrapper => efi_set_bootdev ==> efi_dp_from_name ===> efi_dp_from_part ====> dp_part_size
With result for blk_get_device_part_str dev="ubi" devnr="0:" path="/boot.scr.uimg"
desc =00000000 part=0
And the crash occurs in the function dp_part_size / called from efi_dp_from_part
In trace : pc : [<ffc91d28>] lr : [<ffc92383>] reloc pc : [<c0166d28>] lr : [<c0167383>]
with symbol
System.map:c0167358 T efi_dp_is_multi_instance System.map:c0167378 T efi_dp_from_part System.map:c01673a4 T efi_dp_part_node System.map:c01673c8 T efi_dp_from_file
System.map:c0166d22 t dp_part_size System.map:c0166d50 t dp_part_node
Igor reported a similar bug for a USB device in cmd: fs: fix data abort in load cmd https://lists.denx.de/pipermail/u-boot/2019-April/364484.htmll
I also proposed a protection for the same issue in ums command http://patchwork.ozlabs.org/project/uboot/list/?series=68096
lib/efi_loader/efi_device_path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 53b40c8..fd57be8 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const
char *devnr,
if (!is_net) { part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition, 1);
if (part < 0)
if (part < 0 || !desc)part = 0, desc = NULL occurs for UBI if the UBI file system is mounted.
Returning an error here means in the end that we will not be able to install run GRUB from the UBI device because we cannot describe the boot device.
I think that UBI volumes should be handled like any other block device. This will avoid having separate program paths for UBI and not UBI.
Heiko and Kyungmin could you, please, explain why UBI currently is not providing a struct blk_desc * block descriptor and how this can be fixed.
Best regards
Heinrich
return EFI_INVALID_PARAMETER; if (device)
Best regards
Patrick
On 4/10/19 11:02 AM, Patrick Delaunay wrote:
Check the value of block_dev before to use this pointer.
This patch solves problem for the command "load" when ubifs is previously mounted: in this case the function blk_get_device_part_str("ubi 0") don't return error but return block_dev = NULL and then data abort.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Added to efi-2019-07 branch.
To reproduce the issue, I have a boot script 'boot.scr.uimg' with a load command executed during ubi boot:
load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
I have a data abort for call stack:
- do_load_wrapper for "ubi 0"
-- efi_set_bootdev --- efi_dp_from_name
=> desc = 0 and data abort for access to 'desc->*'
I also proposed a protection for the same issue in ums command http://patchwork.ozlabs.org/project/uboot/list/?series=68096
lib/efi_loader/efi_device_path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 53b40c8..fd57be8 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (!is_net) { part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition, 1);
if (part < 0)
if (part < 0 || !desc) return EFI_INVALID_PARAMETER;if (device)
Handle virtual address in efi_mem_carve_out function when a new region is created to avoid issue in EFI memory map.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
Issue detected during test of test of UEFI on ev1/dk2 boards.
For debug purpose, I add the dump of the map in efi_mem_sort:
static void efi_mem_sort(void) { ..... int i = 0; printf("-- %s -------------------\n", __func__ ); list_for_each(lhandle, &efi_mem) { struct efi_mem_list *lmem; struct efi_mem_desc *cur;
lmem = list_entry(lhandle, struct efi_mem_list, link); cur = &lmem->desc;
printf("%02d: va %08x pa %08x size %08x (%04x pages) => va end %08x attribute = %llx type=%d\n", i++, (u32)cur->virtual_start, (u32)cur->physical_start, (u32)(cur->num_pages << EFI_PAGE_SHIFT), (u32)cur->num_pages, (u32) (cur->virtual_start + (cur->num_pages << EFI_PAGE_SHIFT)), cur->attribute, cur->type); } }
I get the memory layout (for EV1):
00: va fcf87000 pa fff92000 size 0006e000 (006e pages) => va end fcff5000 attribute = 8 type=2 01: va fff91000 pa fff91000 size 00001000 (0001 pages) => va end fff92000 attribute = 8000000000000008 type=5 02: va fcf87000 pa fcf87000 size 0300a000 (300a pages) => va end fff91000 attribute = 8 type=2 03: va fcf86000 pa fcf86000 size 00001000 (0001 pages) => va end fcf87000 attribute = 8000000000000008 type=6 04: va fcf46000 pa fcf7d000 size 00009000 (0009 pages) => va end fcf4f000 attribute = 8 type=0 05: va fcf7c000 pa fcf7c000 size 00001000 (0001 pages) => va end fcf7d000 attribute = 8000000000000008 type=6 06: va fcf46000 pa fcf77000 size 00005000 (0005 pages) => va end fcf4b000 attribute = 8 type=0 07: va c0000000 pa c0000000 size 3cf77000 (3cf77 pages) => va end fcf77000 attribute = 8 type=7
But for some region virtual address (va) != physical address (pa)
After check of the code, the virtual address is not correctly managed when new region is created.
I don't sure that could be a issue, but I solve the issue with the proposed patch:
00: va fff92000 pa fff92000 size 0006e000 (006e pages) => va end 00000000 attribute = 8 type=2 01: va fff91000 pa fff91000 size 00001000 (0001 pages) => va end fff92000 attribute = 8000000000000008 type=5 02: va fcf87000 pa fcf87000 size 0300a000 (300a pages) => va end fff91000 attribute = 8 type=2 03: va fcf86000 pa fcf86000 size 00001000 (0001 pages) => va end fcf87000 attribute = 8000000000000008 type=6 04: va fcf7d000 pa fcf7d000 size 00009000 (0009 pages) => va end fcf86000 attribute = 8 type=0 05: va fcf7c000 pa fcf7c000 size 00001000 (0001 pages) => va end fcf7d000 attribute = 8000000000000008 type=6 06: va fcf77000 pa fcf77000 size 00005000 (0005 pages) => va end fcf7c000 attribute = 8 type=0 07: va c0000000 pa c0000000 size 3cf77000 (3cf77 pages) => va end fcf77000 attribute = 8 type=7
The virtual address are now correct.
PS: I don't found more elegant code to update the virtual address but I avoid to force virtual_address = physical address in this function.
lib/efi_loader/efi_memory.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 81dc5fc..a47adef 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -168,6 +168,16 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, free(map); } else { map->desc.physical_start = carve_end; + if (carve_end == map_end) + map->desc.virtual_start = + map_desc->virtual_start + + (map_desc->num_pages << EFI_PAGE_SHIFT); + else + map->desc.virtual_start = + carve_desc->virtual_start + + (carve_desc->num_pages << + EFI_PAGE_SHIFT); + map->desc.num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; } @@ -186,6 +196,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, newmap = calloc(1, sizeof(*newmap)); newmap->desc = map->desc; newmap->desc.physical_start = carve_start; + if (carve_start == map_start) + newmap->desc.virtual_start = map_desc->virtual_start; + else + newmap->desc.virtual_start = carve_desc->virtual_start; + newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);
On 4/10/19 11:02 AM, Patrick Delaunay wrote:
Handle virtual address in efi_mem_carve_out function when a new region is created to avoid issue in EFI memory map.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Issue detected during test of test of UEFI on ev1/dk2 boards.
For debug purpose, I add the dump of the map in efi_mem_sort:
static void efi_mem_sort(void) { ..... int i = 0; printf("-- %s -------------------\n", __func__ ); list_for_each(lhandle, &efi_mem) { struct efi_mem_list *lmem; struct efi_mem_desc *cur;
lmem = list_entry(lhandle, struct efi_mem_list, link); cur = &lmem->desc; printf("%02d: va %08x pa %08x size %08x (%04x pages) => va end %08x attribute = %llx type=%d\n", i++, (u32)cur->virtual_start, (u32)cur->physical_start, (u32)(cur->num_pages << EFI_PAGE_SHIFT), (u32)cur->num_pages, (u32) (cur->virtual_start + (cur->num_pages << EFI_PAGE_SHIFT)), cur->attribute, cur->type);} }
I get the memory layout (for EV1):
00: va fcf87000 pa fff92000 size 0006e000 (006e pages) => va end fcff5000 attribute = 8 type=2 01: va fff91000 pa fff91000 size 00001000 (0001 pages) => va end fff92000 attribute = 8000000000000008 type=5 02: va fcf87000 pa fcf87000 size 0300a000 (300a pages) => va end fff91000 attribute = 8 type=2 03: va fcf86000 pa fcf86000 size 00001000 (0001 pages) => va end fcf87000 attribute = 8000000000000008 type=6 04: va fcf46000 pa fcf7d000 size 00009000 (0009 pages) => va end fcf4f000 attribute = 8 type=0 05: va fcf7c000 pa fcf7c000 size 00001000 (0001 pages) => va end fcf7d000 attribute = 8000000000000008 type=6 06: va fcf46000 pa fcf77000 size 00005000 (0005 pages) => va end fcf4b000 attribute = 8 type=0 07: va c0000000 pa c0000000 size 3cf77000 (3cf77 pages) => va end fcf77000 attribute = 8 type=7
But for some region virtual address (va) != physical address (pa)
After check of the code, the virtual address is not correctly managed when new region is created.
I don't sure that could be a issue, but I solve the issue with the proposed patch:
00: va fff92000 pa fff92000 size 0006e000 (006e pages) => va end 00000000 attribute = 8 type=2 01: va fff91000 pa fff91000 size 00001000 (0001 pages) => va end fff92000 attribute = 8000000000000008 type=5 02: va fcf87000 pa fcf87000 size 0300a000 (300a pages) => va end fff91000 attribute = 8 type=2 03: va fcf86000 pa fcf86000 size 00001000 (0001 pages) => va end fcf87000 attribute = 8000000000000008 type=6 04: va fcf7d000 pa fcf7d000 size 00009000 (0009 pages) => va end fcf86000 attribute = 8 type=0 05: va fcf7c000 pa fcf7c000 size 00001000 (0001 pages) => va end fcf7d000 attribute = 8000000000000008 type=6 06: va fcf77000 pa fcf77000 size 00005000 (0005 pages) => va end fcf7c000 attribute = 8 type=0 07: va c0000000 pa c0000000 size 3cf77000 (3cf77 pages) => va end fcf77000 attribute = 8 type=7
The virtual address are now correct.
PS: I don't found more elegant code to update the virtual address but I avoid to force virtual_address = physical address in this function.
lib/efi_loader/efi_memory.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 81dc5fc..a47adef 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -168,6 +168,16 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, free(map); } else { map->desc.physical_start = carve_end;
Thanks for reporting the issue. I think you correctly identified the two places where the update of the virtual address is missing.
The efi_mem_carve_out() function is called efi_add_memory_map() which is only called at boottime. By definition the physical and virtual addresses must be the same at boottime. So a single line
map->desc.virtual_start = carve_end;
should be enough here.
if (carve_end == map_end)map->desc.virtual_start =map_desc->virtual_start +(map_desc->num_pages << EFI_PAGE_SHIFT);elsemap->desc.virtual_start =carve_desc->virtual_start +(carve_desc->num_pages <<EFI_PAGE_SHIFT);map->desc.num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT;@@ -186,6 +196,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, newmap = calloc(1, sizeof(*newmap)); newmap->desc = map->desc; newmap->desc.physical_start = carve_start;
newmap->desc.virtual_start = carve_start;
should be ok here.
Best regards
Heinrich
- if (carve_start == map_start)
newmap->desc.virtual_start = map_desc->virtual_start;- else
newmap->desc.virtual_start = carve_desc->virtual_start;- newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);
On Wed, Apr 10, 2019 at 11:02:57AM +0200, Patrick Delaunay wrote:
Avoid ram_end = 0 on 32bit targets with ram_end at 4G.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
example of issue in stm32mp1 board ev1: ram_start = c0000000 size = 40000000 ram_end = 100000000 ram_end &= ~EFI_PAGE_MASK => result is 0
lib/efi_loader/efi_memory.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 55622d2..81dc5fc 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -574,6 +574,10 @@ __weak void efi_add_known_memory(void)
/* Remove partial pages */ ram_end &= ~EFI_PAGE_MASK;
/* Fix for 32bit targets with ram_top at 4G */if (!ram_end)ram_end = 0x100000000ULL;ram_start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
if (ram_end <= ram_start) {
-- 2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi,
I had the exact same issue yesterday and posted a diff which I think is a better approach:
https://patchwork.ozlabs.org/patch/1082739/
Best regards, another Patrick
Hi Patrick,
From: Patrick Wildt patrick@blueri.se
On Wed, Apr 10, 2019 at 11:02:57AM +0200, Patrick Delaunay wrote:
Avoid ram_end = 0 on 32bit targets with ram_end at 4G.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
example of issue in stm32mp1 board ev1: ram_start = c0000000 size = 40000000 ram_end = 100000000 ram_end &= ~EFI_PAGE_MASK => result is 0
lib/efi_loader/efi_memory.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 55622d2..81dc5fc 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -574,6 +574,10 @@ __weak void efi_add_known_memory(void)
/* Remove partial pages */ ram_end &= ~EFI_PAGE_MASK;
/* Fix for 32bit targets with ram_top at 4G */if (!ram_end)ram_end = 0x100000000ULL;- ram_start = (ram_start + EFI_PAGE_MASK) &
~EFI_PAGE_MASK;
if (ram_end <= ram_start) {-- 2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi,
I had the exact same issue yesterday and posted a diff which I think is a better approach:
I tested your patch and yes that also correct my issue.
And I am agree that it is a better approach: So I will hack your patch and I abandon this patch (1/3) of this serie.
Regards
Patrick
https://patchwork.ozlabs.org/patch/1082739/
Best regards, another Patrick
participants (4)
-
Heinrich Schuchardt -
Patrick DELAUNAY -
Patrick Delaunay
-
Patrick Wildt