[U-Boot,v12,3/6] sandbox: smbios: Update to support sandbox

Message ID 20181106225744.139945-4-sjg@chromium.org
State New
Delegated to: Alexander Graf
Headers show
Series
  • efi_loader: Code refactoring and improvement
Related show

Commit Message

Simon Glass Nov. 6, 2018, 10:57 p.m.
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 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                | 32 ++++++++++++++++++++++++--------
 2 files changed, 37 insertions(+), 15 deletions(-)

Comments

Alexander Graf Nov. 13, 2018, 8:03 p.m. | #1
On 06.11.18 23:57, 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 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                | 32 ++++++++++++++++++++++++--------
>  2 files changed, 37 insertions(+), 15 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..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);

This will not work. In this function, you have a variable called
"tables" which now gets an "address" rather than a pointer. It writes
that value to se->struct_table_address later on though which is shared
with (binary) payloads which expect it to be a pointer.

Please convert that one as well :).


Alex

> @@ -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;
>  }
>

Patch

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;
 }