| Message ID | 20181015141750.56480-4-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi_loader: Code refactoring and improvement | expand |
On 15.10.18 16:17, Simon Glass wrote: > At present this code casts addresses to pointers so cannot be used with > sandbox. Update it to use mapmem instead. > > Signed-off-by: Simon Glass <sjg@chromium.org> Unfortunately this won't work. The SMBIOS2 structure itself contains a physical pointer to the target address (which in EFI lands really has to be linear physical pointer). This pointer gets set based on "addr" in write_smbios_table(): tables = addr; [...] se->struct_table_address = tables; So I think the only thing we can do for now is to just graciously fail SMBIOS generation (maybe only on sandbox?) when we can not find a pointer that is < U32_MAX. The shortcoming above was fixed with SMBIOS3, so the "good" path forward would be to add SMBIOS3 support and just not rely on 32bit pointers at all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers to the tables. Depending on that we can either use your maps or we can't. Alex
Hi Alex, On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote: > > > On 15.10.18 16:17, Simon Glass wrote: >> At present this code casts addresses to pointers so cannot be used with >> sandbox. Update it to use mapmem instead. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > > Unfortunately this won't work. The SMBIOS2 structure itself contains a > physical pointer to the target address (which in EFI lands really has to > be linear physical pointer). This pointer gets set based on "addr" in > write_smbios_table(): > > tables = addr; > [...] > se->struct_table_address = tables; Does that actually matter? We will never actually boot anything on sandbox that will use that address. Also sandbox addresses are always <4GB (they start at 0). > > So I think the only thing we can do for now is to just graciously fail > SMBIOS generation (maybe only on sandbox?) when we can not find a > pointer that is < U32_MAX. > > The shortcoming above was fixed with SMBIOS3, so the "good" path forward > would be to add SMBIOS3 support and just not rely on 32bit pointers at > all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers > to the tables. Depending on that we can either use your maps or we can't. Maybe I prefer device tree as it avoid this sort of thing :-) Regards, Simon
On 19.10.18 05:25, Simon Glass wrote: > Hi Alex, > > On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote: >> >> >> On 15.10.18 16:17, Simon Glass wrote: >>> At present this code casts addresses to pointers so cannot be used with >>> sandbox. Update it to use mapmem instead. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >> Unfortunately this won't work. The SMBIOS2 structure itself contains a >> physical pointer to the target address (which in EFI lands really has to >> be linear physical pointer). This pointer gets set based on "addr" in >> write_smbios_table(): >> >> tables = addr; >> [...] >> se->struct_table_address = tables; > > Does that actually matter? We will never actually boot anything on > sandbox that will use that address. Why not? We can boot the UEFI Shell today - and that can use the "address". > Also sandbox addresses are always <4GB (they start at 0). U-Boot addresses are <4GB but pointers are >4GB. U-Boot addresses are also a U-Boot internal concept. We don't have any means to expose their semantics to UEFI applications. So anything inside a data structure that is shared with UEFI that says "address" really means "pointer". So since any UEFI application that we execute is only aware of pointers, we can not represent the pointer in the field. And that breaks every consumer of it. > >> >> So I think the only thing we can do for now is to just graciously fail >> SMBIOS generation (maybe only on sandbox?) when we can not find a >> pointer that is < U32_MAX. >> >> The shortcoming above was fixed with SMBIOS3, so the "good" path forward >> would be to add SMBIOS3 support and just not rely on 32bit pointers at >> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers >> to the tables. Depending on that we can either use your maps or we can't. > > Maybe I prefer device tree as it avoid this sort of thing :-) DT is orthogonal to SMBIOS. SMBIOS describes OEM information, such as "chassy name", "how DIMM slots are populated", etc. Sure, you could start and map SMBIOS information into a Linux specific DT node, but what's the point if we have SMBIOS already ;). Alex
Hi Alex, On 19 October 2018 at 01:27, Alexander Graf <agraf@suse.de> wrote: > > > On 19.10.18 05:25, Simon Glass wrote: >> Hi Alex, >> >> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote: >>> >>> >>> On 15.10.18 16:17, Simon Glass wrote: >>>> At present this code casts addresses to pointers so cannot be used with >>>> sandbox. Update it to use mapmem instead. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> >>> Unfortunately this won't work. The SMBIOS2 structure itself contains a >>> physical pointer to the target address (which in EFI lands really has to >>> be linear physical pointer). This pointer gets set based on "addr" in >>> write_smbios_table(): >>> >>> tables = addr; >>> [...] >>> se->struct_table_address = tables; >> >> Does that actually matter? We will never actually boot anything on >> sandbox that will use that address. > > Why not? We can boot the UEFI Shell today - and that can use the "address". OK, so UEFI shell uses the SMBIOS tables? I didn't know that. > >> Also sandbox addresses are always <4GB (they start at 0). > > U-Boot addresses are <4GB but pointers are >4GB. Actually I have applied a patch to fix that. > > U-Boot addresses are also a U-Boot internal concept. We don't have any > means to expose their semantics to UEFI applications. So anything inside > a data structure that is shared with UEFI that says "address" really > means "pointer". > > So since any UEFI application that we execute is only aware of pointers, > we can not represent the pointer in the field. And that breaks every > consumer of it. Yes we must use pointers in this case, I agree. This needs a map_sysmem() call and a check that it does not overflow. > >> >>> >>> So I think the only thing we can do for now is to just graciously fail >>> SMBIOS generation (maybe only on sandbox?) when we can not find a >>> pointer that is < U32_MAX. >>> >>> The shortcoming above was fixed with SMBIOS3, so the "good" path forward >>> would be to add SMBIOS3 support and just not rely on 32bit pointers at >>> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers >>> to the tables. Depending on that we can either use your maps or we can't. >> >> Maybe I prefer device tree as it avoid this sort of thing :-) > > DT is orthogonal to SMBIOS. SMBIOS describes OEM information, such as > "chassy name", "how DIMM slots are populated", etc. > > Sure, you could start and map SMBIOS information into a Linux specific > DT node, but what's the point if we have SMBIOS already ;). I'm not suggesting we do it, just whining about these legacy data structures :-) Regards, Simon
> Am 22.10.2018 um 18:49 schrieb Simon Glass <sjg@chromium.org>: > > Hi Alex, > >> On 19 October 2018 at 01:27, Alexander Graf <agraf@suse.de> wrote: >> >> >>> On 19.10.18 05:25, Simon Glass wrote: >>> Hi Alex, >>> >>>> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote: >>>> >>>> >>>>> On 15.10.18 16:17, Simon Glass wrote: >>>>> At present this code casts addresses to pointers so cannot be used with >>>>> sandbox. Update it to use mapmem instead. >>>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> >>>> Unfortunately this won't work. The SMBIOS2 structure itself contains a >>>> physical pointer to the target address (which in EFI lands really has to >>>> be linear physical pointer). This pointer gets set based on "addr" in >>>> write_smbios_table(): >>>> >>>> tables = addr; >>>> [...] >>>> se->struct_table_address = tables; >>> >>> Does that actually matter? We will never actually boot anything on >>> sandbox that will use that address. >> >> Why not? We can boot the UEFI Shell today - and that can use the "address". > > OK, so UEFI shell uses the SMBIOS tables? I didn't know that. There is a command to print the contents of smbios tables - and that one does use them :). > >> >>> Also sandbox addresses are always <4GB (they start at 0). >> >> U-Boot addresses are <4GB but pointers are >4GB. > > Actually I have applied a patch to fix that. > >> >> U-Boot addresses are also a U-Boot internal concept. We don't have any >> means to expose their semantics to UEFI applications. So anything inside >> a data structure that is shared with UEFI that says "address" really >> means "pointer". >> >> So since any UEFI application that we execute is only aware of pointers, >> we can not represent the pointer in the field. And that breaks every >> consumer of it. > > Yes we must use pointers in this case, I agree. This needs a > map_sysmem() call and a check that it does not overflow. > >> >>> >>>> >>>> So I think the only thing we can do for now is to just graciously fail >>>> SMBIOS generation (maybe only on sandbox?) when we can not find a >>>> pointer that is < U32_MAX. >>>> >>>> The shortcoming above was fixed with SMBIOS3, so the "good" path forward >>>> would be to add SMBIOS3 support and just not rely on 32bit pointers at >>>> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers >>>> to the tables. Depending on that we can either use your maps or we can't. >>> >>> Maybe I prefer device tree as it avoid this sort of thing :-) >> >> DT is orthogonal to SMBIOS. SMBIOS describes OEM information, such as >> "chassy name", "how DIMM slots are populated", etc. >> >> Sure, you could start and map SMBIOS information into a Linux specific >> DT node, but what's the point if we have SMBIOS already ;). > > I'm not suggesting we do it, just whining about these legacy data structures :-) :) Alex > > Regards, > Simon
Hi Alex, On 22 October 2018 at 12:32, Alexander Graf <agraf@suse.de> wrote: > > > > > Am 22.10.2018 um 18:49 schrieb Simon Glass <sjg@chromium.org>: > > > > Hi Alex, > > > >> On 19 October 2018 at 01:27, Alexander Graf <agraf@suse.de> wrote: > >> > >> > >>> On 19.10.18 05:25, Simon Glass wrote: > >>> Hi Alex, > >>> > >>>> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote: > >>>> > >>>> > >>>>> On 15.10.18 16:17, Simon Glass wrote: > >>>>> At present this code casts addresses to pointers so cannot be used with > >>>>> sandbox. Update it to use mapmem instead. > >>>>> > >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> > >>>> > >>>> Unfortunately this won't work. The SMBIOS2 structure itself contains a > >>>> physical pointer to the target address (which in EFI lands really has to > >>>> be linear physical pointer). This pointer gets set based on "addr" in > >>>> write_smbios_table(): > >>>> > >>>> tables = addr; > >>>> [...] > >>>> se->struct_table_address = tables; > >>> > >>> Does that actually matter? We will never actually boot anything on > >>> sandbox that will use that address. > >> > >> Why not? We can boot the UEFI Shell today - and that can use the "address". > > > > OK, so UEFI shell uses the SMBIOS tables? I didn't know that. > > There is a command to print the contents of smbios tables - and that one does use them :). OK, but it should work without any trouble due to my patch in dm/next. This seems like another reason to make sandbox use 32-bit pointers where possible. Regards, Simon
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 38e42fa2432..a81488495e2 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -7,6 +7,7 @@ #include <common.h> #include <efi_loader.h> +#include <mapmem.h> #include <smbios.h> static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; @@ -19,17 +20,19 @@ static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; efi_status_t efi_smbios_register(void) { /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */ - u64 dmi = U32_MAX; + u64 dmi_addr = U32_MAX; efi_status_t ret; + void *dmi; /* Reserve 4kiB page for SMBIOS */ ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, - EFI_RUNTIME_SERVICES_DATA, 1, &dmi); + EFI_RUNTIME_SERVICES_DATA, 1, &dmi_addr); if (ret != EFI_SUCCESS) { /* Could not find space in lowmem, use highmem instead */ ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, - EFI_RUNTIME_SERVICES_DATA, 1, &dmi); + EFI_RUNTIME_SERVICES_DATA, 1, + &dmi_addr); if (ret != EFI_SUCCESS) return ret; @@ -39,11 +42,14 @@ efi_status_t efi_smbios_register(void) * Generate SMBIOS tables - we know that efi_allocate_pages() returns * a 4k-aligned address, so it is safe to assume that * write_smbios_table() will write the table at that address. + * + * Note that on sandbox, efi_allocate_pages() unfortunately returns a + * pointer even though it uses a uint64_t type. Convert it. */ - assert(!(dmi & 0xf)); - write_smbios_table(dmi); + assert(!(dmi_addr & 0xf)); + dmi = (void *)(uintptr_t)dmi_addr; + write_smbios_table(map_to_sysmem(dmi)); /* And expose them to our EFI payload */ - return efi_install_configuration_table(&smbios_guid, - (void *)(uintptr_t)dmi); + return efi_install_configuration_table(&smbios_guid, dmi); } diff --git a/lib/smbios.c b/lib/smbios.c index 326eb00230d..87109d431a2 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -6,6 +6,7 @@ */ #include <common.h> +#include <mapmem.h> #include <smbios.h> #include <tables_csum.h> #include <version.h> @@ -72,9 +73,10 @@ static int smbios_string_table_len(char *start) static int smbios_write_type0(ulong *current, int handle) { - struct smbios_type0 *t = (struct smbios_type0 *)*current; + struct smbios_type0 *t; int len = sizeof(struct smbios_type0); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type0)); fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); t->vendor = smbios_add_string(t->eos, "U-Boot"); @@ -101,16 +103,18 @@ static int smbios_write_type0(ulong *current, int handle) len = t->length + smbios_string_table_len(t->eos); *current += len; + unmap_sysmem(t); return len; } static int smbios_write_type1(ulong *current, int handle) { - struct smbios_type1 *t = (struct smbios_type1 *)*current; + struct smbios_type1 *t; int len = sizeof(struct smbios_type1); char *serial_str = env_get("serial#"); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type1)); fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER); @@ -122,15 +126,17 @@ static int smbios_write_type1(ulong *current, int handle) len = t->length + smbios_string_table_len(t->eos); *current += len; + unmap_sysmem(t); return len; } static int smbios_write_type2(ulong *current, int handle) { - struct smbios_type2 *t = (struct smbios_type2 *)*current; + struct smbios_type2 *t; int len = sizeof(struct smbios_type2); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type2)); fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER); @@ -140,15 +146,17 @@ static int smbios_write_type2(ulong *current, int handle) len = t->length + smbios_string_table_len(t->eos); *current += len; + unmap_sysmem(t); return len; } static int smbios_write_type3(ulong *current, int handle) { - struct smbios_type3 *t = (struct smbios_type3 *)*current; + struct smbios_type3 *t; int len = sizeof(struct smbios_type3); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type3)); fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER); @@ -160,6 +168,7 @@ static int smbios_write_type3(ulong *current, int handle) len = t->length + smbios_string_table_len(t->eos); *current += len; + unmap_sysmem(t); return len; } @@ -198,9 +207,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t) static int smbios_write_type4(ulong *current, int handle) { - struct smbios_type4 *t = (struct smbios_type4 *)*current; + struct smbios_type4 *t; int len = sizeof(struct smbios_type4); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type4)); fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle); t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL; @@ -214,32 +224,37 @@ static int smbios_write_type4(ulong *current, int handle) len = t->length + smbios_string_table_len(t->eos); *current += len; + unmap_sysmem(t); return len; } static int smbios_write_type32(ulong *current, int handle) { - struct smbios_type32 *t = (struct smbios_type32 *)*current; + struct smbios_type32 *t; int len = sizeof(struct smbios_type32); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type32)); fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle); *current += len; + unmap_sysmem(t); return len; } static int smbios_write_type127(ulong *current, int handle) { - struct smbios_type127 *t = (struct smbios_type127 *)*current; + struct smbios_type127 *t; int len = sizeof(struct smbios_type127); + t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type127)); fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle); *current += len; + unmap_sysmem(t); return len; } @@ -268,7 +283,7 @@ ulong write_smbios_table(ulong addr) /* 16 byte align the table address */ addr = ALIGN(addr, 16); - se = (struct smbios_entry *)(uintptr_t)addr; + se = map_sysmem(addr, sizeof(struct smbios_entry)); memset(se, 0, sizeof(struct smbios_entry)); addr += sizeof(struct smbios_entry); @@ -298,6 +313,7 @@ ulong write_smbios_table(ulong addr) isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET; se->intermediate_checksum = table_compute_checksum(istart, isize); se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry)); + unmap_sysmem(se); return addr; }
At present this code casts addresses to pointers so cannot be used with sandbox. Update it to use mapmem instead. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v11: - Fix the EFI code that has since been added and relies on broken behaviour Changes in v9: None Changes in v7: None Changes in v5: None Changes in v4: None Changes in v3: - Drop incorrect map_sysmem() in write_smbios_table() lib/efi_loader/efi_smbios.c | 20 +++++++++++++------- lib/smbios.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 37 insertions(+), 15 deletions(-)