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

[Alex Hung]

On 2017-07-12 05:53 AM, Colin King wrote:
> From: Colin Ian King <colin.king at 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 at 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 at canonical.com>