Message ID | 20181114065043.241424-2-sjg@chromium.org |
---|---|
State | Superseded |
Delegated to: | Alexander Graf |
Headers | show |
Series | efi_loader: Code refactoring and improvement | expand |
On 11/14/2018 07:50 AM, 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> > --- > > Changes in v13: > - Update code to deal with the struct_table_address member > > Changes in v12: None > 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 | 52 ++++++++++++++++++++++++++++++------- > 2 files changed, 56 insertions(+), 16 deletions(-) > > 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..2cd08b5e16f 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; > } > @@ -257,6 +272,7 @@ static smbios_write_type smbios_write_funcs[] = { > ulong write_smbios_table(ulong addr) > { > struct smbios_entry *se; > + ulong table_addr; > ulong tables; > int len = 0; > int max_struct_size = 0; > @@ -268,7 +284,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); > @@ -290,7 +306,24 @@ ulong write_smbios_table(ulong addr) > se->max_struct_size = max_struct_size; > memcpy(se->intermediate_anchor, "_DMI_", 5); > se->struct_table_length = len; > - se->struct_table_address = tables; > + > + /* > + * We must use a pointer here so things work correctly on sandbox. The > + * user of this table is not aware of the mapping of addresses to > + * sandbox's DRAM buffer. > + */ > + table_addr = (ulong)map_sysmem(tables, 0); > + if (sizeof(table_addr) >= sizeof(u32) && table_addr >= (1ULL << 32)) { sizeof(long) >= sizeof(u32) will always be true on any platform U-Boot supports, no? How about if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) > + /* > + * We need to put this >32-bit pointer into the table but the > + * field is only 32 bits wide. > + */ > + printf("WARNING: SMBIOS table_address overflow %llx\n", > + (unsigned long long)table_addr); > + table_addr = 0; I'm also not sure what to do about the error case. IMHO ideally we should propagate the error to the upper layers, but consider it non-fatal. > + } > + se->struct_table_address = table_addr; > + > se->struct_count = handle; > > /* calculate checksums */ > @@ -298,6 +331,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; > }
Hi Alex, On 14 November 2018 at 02:18, Alexander Graf <agraf@suse.de> wrote: > > On 11/14/2018 07:50 AM, 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> >> --- >> >> Changes in v13: >> - Update code to deal with the struct_table_address member >> >> Changes in v12: None >> 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 | 52 ++++++++++++++++++++++++++++++------- >> 2 files changed, 56 insertions(+), 16 deletions(-) >> [..] >> + >> + /* >> + * We must use a pointer here so things work correctly on sandbox. The >> + * user of this table is not aware of the mapping of addresses to >> + * sandbox's DRAM buffer. >> + */ >> + table_addr = (ulong)map_sysmem(tables, 0); >> + if (sizeof(table_addr) >= sizeof(u32) && table_addr >= (1ULL << 32)) { > > > sizeof(long) >= sizeof(u32) will always be true on any platform U-Boot supports, no? > > How about > > if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) Yes that seems right, thanks. > >> + /* >> + * We need to put this >32-bit pointer into the table but the >> + * field is only 32 bits wide. >> + */ >> + printf("WARNING: SMBIOS table_address overflow %llx\n", >> + (unsigned long long)table_addr); >> + table_addr = 0; > > > I'm also not sure what to do about the error case. IMHO ideally we should propagate the error to the upper layers, but consider it non-fatal. Yes agreed, but it involves changing the function signature of this and everything else in table_write_funcs[] so I decided to leave it alone for now, since the effect is the same. 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..2cd08b5e16f 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; } @@ -257,6 +272,7 @@ static smbios_write_type smbios_write_funcs[] = { ulong write_smbios_table(ulong addr) { struct smbios_entry *se; + ulong table_addr; ulong tables; int len = 0; int max_struct_size = 0; @@ -268,7 +284,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); @@ -290,7 +306,24 @@ ulong write_smbios_table(ulong addr) se->max_struct_size = max_struct_size; memcpy(se->intermediate_anchor, "_DMI_", 5); se->struct_table_length = len; - se->struct_table_address = tables; + + /* + * We must use a pointer here so things work correctly on sandbox. The + * user of this table is not aware of the mapping of addresses to + * sandbox's DRAM buffer. + */ + table_addr = (ulong)map_sysmem(tables, 0); + if (sizeof(table_addr) >= sizeof(u32) && table_addr >= (1ULL << 32)) { + /* + * We need to put this >32-bit pointer into the table but the + * field is only 32 bits wide. + */ + printf("WARNING: SMBIOS table_address overflow %llx\n", + (unsigned long long)table_addr); + table_addr = 0; + } + se->struct_table_address = table_addr; + se->struct_count = handle; /* calculate checksums */ @@ -298,6 +331,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 v13: - Update code to deal with the struct_table_address member Changes in v12: None 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 | 52 ++++++++++++++++++++++++++++++------- 2 files changed, 56 insertions(+), 16 deletions(-)