diff mbox

lib: fwts_smbios: use fwts_safe_memcpy to read SMBIOS entry

Message ID 20170811134140.3947-1-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Aug. 11, 2017, 1:41 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Use fwts_safe_memcpy rather than a fwts_safe_memread and then an
un-safe copy as a micro optimisation.  Also throw an error and
return a NULL address if the memcpy failed because the memory is
unreadable.  Fixes a bug on aarch64 systems where this entry is
non-readable and causes a BUS error.

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

Comments

Naresh Bhat Aug. 11, 2017, 3:14 p.m. UTC | #1 
Thanks Colin,

On 11 August 2017 at 19:11, Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
>
> Use fwts_safe_memcpy rather than a fwts_safe_memread and then an
> un-safe copy as a micro optimisation.  Also throw an error and
> return a NULL address if the memcpy failed because the memory is
> unreadable.  Fixes a bug on aarch64 systems where this entry is
> non-readable and causes a BUS error.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_smbios.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
> index b3773aa2..abef4eb6 100644
> --- a/src/lib/src/fwts_smbios.c
> +++ b/src/lib/src/fwts_smbios.c
> @@ -56,13 +56,14 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework
> *fw, fwts_smbios_entry *
>                 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;
> +                       if (fwts_safe_memcpy(entry, mapped_entry, size) ==
> FWTS_OK) {
>                                 (void)fwts_munmap(mapped_entry, size);
>                                 *type  = FWTS_SMBIOS;
>                                 return addr;
>                         } else {
> +                               fwts_log_error(fw, "Cannot read mmap'd
> SMBIOS entry at 0x%p\n", addr);
>                                 (void)fwts_munmap(mapped_entry, size);
> +                               addr = NULL;
>                         }
>                 }
>
> @@ -73,7 +74,7 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework
> *fw, fwts_smbios_entry *
>                         return addr;
>                 }
>
> -               fwts_log_error(fw, "Cannot mmap SMBIOS entry at %p\n",
> addr);
> +               fwts_log_error(fw, "Cannot mmap SMBIOS entry at 0x%p\n",
> addr);
>         }
>         return NULL;
>  }
> --
> 2.11.0
>

I have tested this patch on Cavium ThunderX2 hardware.

Reviewed-By/Tested-By: Naresh Bhat <naresh.bhat@linaro.org>
Alex Hung Aug. 14, 2017, 6:56 p.m. UTC | #2 
On 2017-08-11 06:41 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Use fwts_safe_memcpy rather than a fwts_safe_memread and then an
> un-safe copy as a micro optimisation.  Also throw an error and
> return a NULL address if the memcpy failed because the memory is
> unreadable.  Fixes a bug on aarch64 systems where this entry is
> non-readable and causes a BUS error.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_smbios.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
> index b3773aa2..abef4eb6 100644
> --- a/src/lib/src/fwts_smbios.c
> +++ b/src/lib/src/fwts_smbios.c
> @@ -56,13 +56,14 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   		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;
> +			if (fwts_safe_memcpy(entry, mapped_entry, size) == FWTS_OK) {
>   				(void)fwts_munmap(mapped_entry, size);
>   				*type  = FWTS_SMBIOS;
>   				return addr;
>   			} else {
> +				fwts_log_error(fw, "Cannot read mmap'd SMBIOS entry at 0x%p\n", addr);
>   				(void)fwts_munmap(mapped_entry, size);
> +				addr = NULL;
>   			}
>   		}
>   
> @@ -73,7 +74,7 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   			return addr;
>   		}
>   
> -		fwts_log_error(fw, "Cannot mmap SMBIOS entry at %p\n", addr);
> +		fwts_log_error(fw, "Cannot mmap SMBIOS entry at 0x%p\n", addr);
>   	}
>   	return NULL;
>   }
>
Ivan Hu Aug. 21, 2017, 6:40 a.m. UTC | #3 
On 08/11/2017 09:41 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Use fwts_safe_memcpy rather than a fwts_safe_memread and then an
> un-safe copy as a micro optimisation.  Also throw an error and
> return a NULL address if the memcpy failed because the memory is
> unreadable.  Fixes a bug on aarch64 systems where this entry is
> non-readable and causes a BUS error.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_smbios.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
> index b3773aa2..abef4eb6 100644
> --- a/src/lib/src/fwts_smbios.c
> +++ b/src/lib/src/fwts_smbios.c
> @@ -56,13 +56,14 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   		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;
> +			if (fwts_safe_memcpy(entry, mapped_entry, size) == FWTS_OK) {
>   				(void)fwts_munmap(mapped_entry, size);
>   				*type  = FWTS_SMBIOS;
>   				return addr;
>   			} else {
> +				fwts_log_error(fw, "Cannot read mmap'd SMBIOS entry at 0x%p\n", addr);
>   				(void)fwts_munmap(mapped_entry, size);
> +				addr = NULL;
>   			}
>   		}
>   
> @@ -73,7 +74,7 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   			return addr;
>   		}
>   
> -		fwts_log_error(fw, "Cannot mmap SMBIOS entry at %p\n", addr);
> +		fwts_log_error(fw, "Cannot mmap SMBIOS entry at 0x%p\n", addr);
>   	}
>   	return NULL;
>   }
> 

Acked-by: Ivan Hu <ivan.hu@canonical.com>
Alex Hung Aug. 21, 2017, 6:50 a.m. UTC | #4 
On 2017-08-11 06:41 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Use fwts_safe_memcpy rather than a fwts_safe_memread and then an
> un-safe copy as a micro optimisation.  Also throw an error and
> return a NULL address if the memcpy failed because the memory is
> unreadable.  Fixes a bug on aarch64 systems where this entry is
> non-readable and causes a BUS error.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_smbios.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
> index b3773aa2..abef4eb6 100644
> --- a/src/lib/src/fwts_smbios.c
> +++ b/src/lib/src/fwts_smbios.c
> @@ -56,13 +56,14 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   		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;
> +			if (fwts_safe_memcpy(entry, mapped_entry, size) == FWTS_OK) {
>   				(void)fwts_munmap(mapped_entry, size);
>   				*type  = FWTS_SMBIOS;
>   				return addr;
>   			} else {
> +				fwts_log_error(fw, "Cannot read mmap'd SMBIOS entry at 0x%p\n", addr);
>   				(void)fwts_munmap(mapped_entry, size);
> +				addr = NULL;
>   			}
>   		}
>   
> @@ -73,7 +74,7 @@ static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
>   			return addr;
>   		}
>   
> -		fwts_log_error(fw, "Cannot mmap SMBIOS entry at %p\n", addr);
> +		fwts_log_error(fw, "Cannot mmap SMBIOS entry at 0x%p\n", addr);
>   	}
>   	return NULL;
>   }
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_smbios.c b/src/lib/src/fwts_smbios.c
index b3773aa2..abef4eb6 100644
--- a/src/lib/src/fwts_smbios.c
+++ b/src/lib/src/fwts_smbios.c
@@ -56,13 +56,14 @@  static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
 		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;
+			if (fwts_safe_memcpy(entry, mapped_entry, size) == FWTS_OK) {
 				(void)fwts_munmap(mapped_entry, size);
 				*type  = FWTS_SMBIOS;
 				return addr;
 			} else {
+				fwts_log_error(fw, "Cannot read mmap'd SMBIOS entry at 0x%p\n", addr);
 				(void)fwts_munmap(mapped_entry, size);
+				addr = NULL;
 			}
 		}
 
@@ -73,7 +74,7 @@  static void *fwts_smbios_find_entry_uefi(fwts_framework *fw, fwts_smbios_entry *
 			return addr;
 		}
 
-		fwts_log_error(fw, "Cannot mmap SMBIOS entry at %p\n", addr);
+		fwts_log_error(fw, "Cannot mmap SMBIOS entry at 0x%p\n", addr);
 	}
 	return NULL;
 }