diff mbox

[5/5] lib: fwts_smbios: ensure mmap'd memory is readable before accessing it

Message ID 20170712125334.20176-6-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King July 12, 2017, 12:53 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

We need to check we don't get SIGSEGV or SIGBUS errors when reading
the mmap'd data before we try and access it. Use the fwts_safe_memread
check on the data to sanity check these mappings.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_smbios.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

Comments

Alex Hung July 12, 2017, 3:43 p.m. UTC | #1 
On 2017-07-12 05:53 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> We need to check we don't get SIGSEGV or SIGBUS errors when reading
> the mmap'd data before we try and access it. Use the fwts_safe_memread
> check on the data to sanity check these mappings.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_smbios.c | 36 +++++++++++++++++++++++++-----------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
> index 8d0ea39b..b3773aa2 100644
> --- a/src/lib/src/fwts_smbios.c
> +++ b/src/lib/src/fwts_smbios.c
> @@ -53,12 +53,17 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   
>   	if ((addr = fwts_scan_efi_systab("SMBIOS")) != NULL) {
>   		fwts_smbios_entry *mapped_entry;
> -
> -		if ((mapped_entry = fwts_mmap((off_t)addr, sizeof(fwts_smbios_entry))) != FWTS_MAP_FAILED) {
> -			*entry = *mapped_entry;
> -			(void)fwts_munmap(mapped_entry, sizeof(fwts_smbios_entry));
> -			*type  = FWTS_SMBIOS;
> -			return addr;
> +		const size_t size = sizeof(fwts_smbios_entry);
> +
> +		if ((mapped_entry = fwts_mmap((off_t)addr, size)) != FWTS_MAP_FAILED) {
> +			if (fwts_safe_memread(mapped_entry, size) == FWTS_OK) {
> +				*entry = *mapped_entry;
> +				(void)fwts_munmap(mapped_entry, size);
> +				*type  = FWTS_SMBIOS;
> +				return addr;
> +			} else {
> +				(void)fwts_munmap(mapped_entry, size);
> +			}
>   		}
>   
>   		if (fwts_load_file("/sys/firmware/dmi/tables/smbios_entry_point",
> @@ -83,11 +88,16 @@ static void *fwts_smbios30_find_entry_uefi(fwts_framework *fw, fwts_smbios30_ent
>   
>   	if ((addr = fwts_scan_efi_systab("SMBIOS3")) != NULL) {
>   		fwts_smbios30_entry *mapped_entry;
> -
> -		if ((mapped_entry = fwts_mmap((off_t)addr, sizeof(fwts_smbios30_entry))) != FWTS_MAP_FAILED) {
> -			*entry = *mapped_entry;
> -			(void)fwts_munmap(mapped_entry, sizeof(fwts_smbios30_entry));
> -			return addr;
> +		const size_t size = sizeof(fwts_smbios30_entry);
> +
> +		if ((mapped_entry = fwts_mmap((off_t)addr, size)) != FWTS_MAP_FAILED) {
> +			if (fwts_safe_memread(mapped_entry, size) == FWTS_OK) {
> +				*entry = *mapped_entry;
> +				(void)fwts_munmap(mapped_entry, size);
> +				return addr;
> +			} else {
> +				(void)fwts_munmap(mapped_entry, size);
> +			}
>   		}
>   
>   		if (fwts_load_file("/sys/firmware/dmi/tables/smbios_entry_point",
> @@ -118,6 +128,8 @@ static void *fwts_smbios_find_entry_bios(fwts_framework *fw, fwts_smbios_entry *
>   	}
>   
>   	for (i = 0; i < FWTS_SMBIOS_REGION_SIZE; i += 16) {
> +		if (fwts_safe_memread(mem + i, 16) != FWTS_OK)
> +			continue;
>   		/* SMBIOS entry point */
>   		if ((*(mem+i)   == '_') &&
>   		    (*(mem+i+1) == 'S') &&
> @@ -165,6 +177,8 @@ static void *fwts_smbios30_find_entry_bios(fwts_framework *fw, fwts_smbios30_ent
>   	}
>   
>   	for (i = 0; i < FWTS_SMBIOS_REGION_SIZE; i += 16) {
> +		if (fwts_safe_memread(mem + i, 16) != FWTS_OK)
> +			continue;
>   		/* SMBIOS30 entry point */
>   		if ((*(mem+i)   == '_') &&
>   		    (*(mem+i+1) == 'S') &&
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu July 13, 2017, 2:18 a.m. UTC | #2 
On 07/12/2017 08:53 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> We need to check we don't get SIGSEGV or SIGBUS errors when reading
> the mmap'd data before we try and access it. Use the fwts_safe_memread
> check on the data to sanity check these mappings.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_smbios.c | 36 +++++++++++++++++++++++++-----------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
> index 8d0ea39b..b3773aa2 100644
> --- a/src/lib/src/fwts_smbios.c
> +++ b/src/lib/src/fwts_smbios.c
> @@ -53,12 +53,17 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   
>   	if ((addr = fwts_scan_efi_systab("SMBIOS")) != NULL) {
>   		fwts_smbios_entry *mapped_entry;
> -
> -		if ((mapped_entry = fwts_mmap((off_t)addr, sizeof(fwts_smbios_entry))) != FWTS_MAP_FAILED) {
> -			*entry = *mapped_entry;
> -			(void)fwts_munmap(mapped_entry, sizeof(fwts_smbios_entry));
> -			*type  = FWTS_SMBIOS;
> -			return addr;
> +		const size_t size = sizeof(fwts_smbios_entry);
> +
> +		if ((mapped_entry = fwts_mmap((off_t)addr, size)) != FWTS_MAP_FAILED) {
> +			if (fwts_safe_memread(mapped_entry, size) == FWTS_OK) {
> +				*entry = *mapped_entry;
> +				(void)fwts_munmap(mapped_entry, size);
> +				*type  = FWTS_SMBIOS;
> +				return addr;
> +			} else {
> +				(void)fwts_munmap(mapped_entry, size);
> +			}
>   		}
>   
>   		if (fwts_load_file("/sys/firmware/dmi/tables/smbios_entry_point",
> @@ -83,11 +88,16 @@ static void *fwts_smbios30_find_entry_uefi(fwts_framework *fw, fwts_smbios30_ent
>   
>   	if ((addr = fwts_scan_efi_systab("SMBIOS3")) != NULL) {
>   		fwts_smbios30_entry *mapped_entry;
> -
> -		if ((mapped_entry = fwts_mmap((off_t)addr, sizeof(fwts_smbios30_entry))) != FWTS_MAP_FAILED) {
> -			*entry = *mapped_entry;
> -			(void)fwts_munmap(mapped_entry, sizeof(fwts_smbios30_entry));
> -			return addr;
> +		const size_t size = sizeof(fwts_smbios30_entry);
> +
> +		if ((mapped_entry = fwts_mmap((off_t)addr, size)) != FWTS_MAP_FAILED) {
> +			if (fwts_safe_memread(mapped_entry, size) == FWTS_OK) {
> +				*entry = *mapped_entry;
> +				(void)fwts_munmap(mapped_entry, size);
> +				return addr;
> +			} else {
> +				(void)fwts_munmap(mapped_entry, size);
> +			}
>   		}
>   
>   		if (fwts_load_file("/sys/firmware/dmi/tables/smbios_entry_point",
> @@ -118,6 +128,8 @@ static void *fwts_smbios_find_entry_bios(fwts_framework *fw, fwts_smbios_entry *
>   	}
>   
>   	for (i = 0; i < FWTS_SMBIOS_REGION_SIZE; i += 16) {
> +		if (fwts_safe_memread(mem + i, 16) != FWTS_OK)
> +			continue;
>   		/* SMBIOS entry point */
>   		if ((*(mem+i)   == '_') &&
>   		    (*(mem+i+1) == 'S') &&
> @@ -165,6 +177,8 @@ static void *fwts_smbios30_find_entry_bios(fwts_framework *fw, fwts_smbios30_ent
>   	}
>   
>   	for (i = 0; i < FWTS_SMBIOS_REGION_SIZE; i += 16) {
> +		if (fwts_safe_memread(mem + i, 16) != FWTS_OK)
> +			continue;
>   		/* SMBIOS30 entry point */
>   		if ((*(mem+i)   == '_') &&
>   		    (*(mem+i+1) == 'S') &&
> 

Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
index 8d0ea39b..b3773aa2 100644
--- a/src/lib/src/fwts_smbios.c
+++ b/src/lib/src/fwts_smbios.c
@@ -53,12 +53,17 @@  static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
 
 	if ((addr = fwts_scan_efi_systab("SMBIOS")) != NULL) {
 		fwts_smbios_entry *mapped_entry;
-
-		if ((mapped_entry = fwts_mmap((off_t)addr, sizeof(fwts_smbios_entry))) != FWTS_MAP_FAILED) {
-			*entry = *mapped_entry;
-			(void)fwts_munmap(mapped_entry, sizeof(fwts_smbios_entry));
-			*type  = FWTS_SMBIOS;
-			return addr;
+		const size_t size = sizeof(fwts_smbios_entry);
+
+		if ((mapped_entry = fwts_mmap((off_t)addr, size)) != FWTS_MAP_FAILED) {
+			if (fwts_safe_memread(mapped_entry, size) == FWTS_OK) {
+				*entry = *mapped_entry;
+				(void)fwts_munmap(mapped_entry, size);
+				*type  = FWTS_SMBIOS;
+				return addr;
+			} else {
+				(void)fwts_munmap(mapped_entry, size);
+			}
 		}
 
 		if (fwts_load_file("/sys/firmware/dmi/tables/smbios_entry_point",
@@ -83,11 +88,16 @@  static void *fwts_smbios30_find_entry_uefi(fwts_framework *fw, fwts_smbios30_ent
 
 	if ((addr = fwts_scan_efi_systab("SMBIOS3")) != NULL) {
 		fwts_smbios30_entry *mapped_entry;
-
-		if ((mapped_entry = fwts_mmap((off_t)addr, sizeof(fwts_smbios30_entry))) != FWTS_MAP_FAILED) {
-			*entry = *mapped_entry;
-			(void)fwts_munmap(mapped_entry, sizeof(fwts_smbios30_entry));
-			return addr;
+		const size_t size = sizeof(fwts_smbios30_entry);
+
+		if ((mapped_entry = fwts_mmap((off_t)addr, size)) != FWTS_MAP_FAILED) {
+			if (fwts_safe_memread(mapped_entry, size) == FWTS_OK) {
+				*entry = *mapped_entry;
+				(void)fwts_munmap(mapped_entry, size);
+				return addr;
+			} else {
+				(void)fwts_munmap(mapped_entry, size);
+			}
 		}
 
 		if (fwts_load_file("/sys/firmware/dmi/tables/smbios_entry_point",
@@ -118,6 +128,8 @@  static void *fwts_smbios_find_entry_bios(fwts_framework *fw, fwts_smbios_entry *
 	}
 
 	for (i = 0; i < FWTS_SMBIOS_REGION_SIZE; i += 16) {
+		if (fwts_safe_memread(mem + i, 16) != FWTS_OK)
+			continue;
 		/* SMBIOS entry point */
 		if ((*(mem+i)   == '_') &&
 		    (*(mem+i+1) == 'S') &&
@@ -165,6 +177,8 @@  static void *fwts_smbios30_find_entry_bios(fwts_framework *fw, fwts_smbios30_ent
 	}
 
 	for (i = 0; i < FWTS_SMBIOS_REGION_SIZE; i += 16) {
+		if (fwts_safe_memread(mem + i, 16) != FWTS_OK)
+			continue;
 		/* SMBIOS30 entry point */
 		if ((*(mem+i)   == '_') &&
 		    (*(mem+i+1) == 'S') &&