ACK: [PATCH] lib: fwts: add more sanity checks when fetching the RSDP (LP: #1372849)

Alex Hung alex.hung at canonical.com
Tue Sep 23 23:20:30 UTC 2014


On 14-09-23 06:04 PM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> The --rsdp option allows a user to specify the RSDP address, which
> could be erroneous. Also, it is possible (but very unlikely) that
> the RSDP in set incorrectly at boot time. Currently fwts_acpi_get_rsdp()
> does some checking but fails to return NULL on failure so fwts segfaults.
> Also fwts assumes that the RSDP address is readable, but we should also
> catch any segv faults in case it is not.
>
> This patch adds some "safe" memory helpers that can catch segfaults and
> uses these in the table loading mmap'd reads and copies.  The patch also
> ensures NULL is returned if the RSDP is fails any sanity checks.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>   src/lib/include/fwts.h          |  1 +
>   src/lib/include/fwts_safe_mem.h | 26 +++++++++++++++++
>   src/lib/src/Makefile.am         |  5 ++--
>   src/lib/src/fwts_acpi_tables.c  | 37 +++++++++++++++++++-----
>   src/lib/src/fwts_safe_mem.c     | 63 +++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 123 insertions(+), 9 deletions(-)
>   create mode 100644 src/lib/include/fwts_safe_mem.h
>   create mode 100644 src/lib/src/fwts_safe_mem.c
>
> diff --git a/src/lib/include/fwts.h b/src/lib/include/fwts.h
> index 84075f7..730967d 100644
> --- a/src/lib/include/fwts.h
> +++ b/src/lib/include/fwts.h
> @@ -80,5 +80,6 @@
>   #include "fwts_ioport.h"
>   #include "fwts_release.h"
>   #include "fwts_pci.h"
> +#include "fwts_safe_mem.h"
>   
>   #endif
> diff --git a/src/lib/include/fwts_safe_mem.h b/src/lib/include/fwts_safe_mem.h
> new file mode 100644
> index 0000000..4490026
> --- /dev/null
> +++ b/src/lib/include/fwts_safe_mem.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (C) 2014 Canonical
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +
> +#ifndef __FWTS_SAFE_MEMCPY_H__
> +#define __FWTS_SAFE_MEMCPY_H__
> +
> +int fwts_safe_memcpy(void *dst, const void *src, const size_t n);
> +int fwts_safe_memread(const void *src, const size_t n);
> +
> +#endif
> diff --git a/src/lib/src/Makefile.am b/src/lib/src/Makefile.am
> index 1385c68..fca9c06 100644
> --- a/src/lib/src/Makefile.am
> +++ b/src/lib/src/Makefile.am
> @@ -72,5 +72,6 @@ libfwts_la_SOURCES = 		\
>   	fwts_text_list.c 	\
>   	fwts_tty.c 		\
>   	fwts_uefi.c 		\
> -	fwts_wakealarm.c \
> -	fwts_pm_method.c
> +	fwts_wakealarm.c 	\
> +	fwts_pm_method.c	\
> +	fwts_safe_mem.c
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index 343f849..56498e0 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -112,9 +112,16 @@ static void *fwts_acpi_find_rsdp_bios(void)
>   	/* Scan BIOS for RSDP, ACPI spec states it is aligned on 16 byte intervals */
>   	for (ptr = bios; ptr < (bios+BIOS_LENGTH); ptr += 16) {
>   		rsdp = (fwts_acpi_table_rsdp*)ptr;
> +
> +		/* Can we read this memory w/o segfaulting? */
> +		if (fwts_safe_memread(rsdp, 8) != FWTS_OK)
> +			continue;
> +
>   		/* Look for RSD PTR string */
>   		if (strncmp(rsdp->signature, "RSD PTR ",8) == 0) {
>   			int length = (rsdp->revision < 1) ? 20 : 36;
> +			if (fwts_safe_memread(ptr, length) != FWTS_OK)
> +				continue;
>   			if (fwts_checksum(ptr, length) == 0) {
>   				addr = (void*)(BIOS_START+(ptr - bios));
>   				break;
> @@ -134,7 +141,7 @@ static void *fwts_acpi_find_rsdp_bios(void)
>    *	given the address of the rsdp, map in the region, copy it and
>    *	return the rsdp table. Return NULL if fails.
>    */
> -static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(void *addr, size_t *rsdp_len)
> +static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(fwts_framework *fw, void *addr, size_t *rsdp_len)
>   {
>   	uint8_t *mem;
>   	fwts_acpi_table_rsdp *rsdp = NULL;
> @@ -143,17 +150,27 @@ static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(void *addr, size_t *rsdp_len)
>   	if ((mem = fwts_mmap((off_t)addr, sizeof(fwts_acpi_table_rsdp))) == FWTS_MAP_FAILED)
>   		return NULL;
>   
> +	if (fwts_safe_memread(mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
> +		fwts_log_error(fw, "Cannot safely read RSDP from address %p.", mem);
> +		goto out;
> +	}
> +
>   	rsdp = (fwts_acpi_table_rsdp*)mem;
>   	/* Determine original RSDP size from revision. */
>   	*rsdp_len = (rsdp->revision < 1) ? 20 : 36;
>   
>   	/* Must have the correct signature */
> -	if (strncmp(rsdp->signature, "RSD PTR ", 8))
> +	if (strncmp(rsdp->signature, "RSD PTR ", 8)) {
> +		fwts_log_error(fw, "RSDP did not have expected signature.");
> +		rsdp = NULL;
>   		goto out;
> +	}
>   
>   	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
> -	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL)
> +	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
> +		rsdp = NULL;
>   		goto out;
> +	}
>   
>   	memcpy(rsdp, mem, *rsdp_len);
>   out:
> @@ -177,6 +194,11 @@ static void *fwts_acpi_load_table(const off_t addr)
>   	if ((hdr = fwts_mmap((off_t)addr, sizeof(fwts_acpi_table_header))) == FWTS_MAP_FAILED)
>   		return NULL;
>   
> +	if (fwts_safe_memread(hdr, sizeof(fwts_acpi_table_header)) != FWTS_OK) {
> +		(void)fwts_munmap(hdr, sizeof(fwts_acpi_table_header));
> +		return NULL;
> +	}
> +
>   	len = hdr->length;
>   	if (len < (int)sizeof(fwts_acpi_table_header)) {
>   		(void)fwts_munmap(hdr, sizeof(fwts_acpi_table_header));
> @@ -187,11 +209,12 @@ static void *fwts_acpi_load_table(const off_t addr)
>   
>   	if ((table = fwts_low_calloc(1, len)) == NULL)
>   		return NULL;
> -
>   	if ((mem = fwts_mmap((off_t)addr, len)) == FWTS_MAP_FAILED)
>   		return NULL;
> -
> -	memcpy(table, mem, len);
> +	if (fwts_safe_memcpy(table, mem, len) != FWTS_OK) {
> +		(void)fwts_munmap(mem, len);
> +		return NULL;
> +	}
>   	(void)fwts_munmap(mem, len);
>   
>   	return table;
> @@ -392,7 +415,7 @@ static int fwts_acpi_load_tables_from_firmware(fwts_framework *fw)
>   		return FWTS_ERROR;
>   
>   	/* Load and save cached RSDP */
> -	if ((rsdp = fwts_acpi_get_rsdp(rsdp_addr, &rsdp_len)) == NULL)
> +	if ((rsdp = fwts_acpi_get_rsdp(fw, rsdp_addr, &rsdp_len)) == NULL)
>   		return FWTS_ERROR;
>   
>   	fwts_acpi_add_table("RSDP", rsdp, (uint64_t)(off_t)rsdp_addr, rsdp_len, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c
> new file mode 100644
> index 0000000..6fcfe1f
> --- /dev/null
> +++ b/src/lib/src/fwts_safe_mem.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2014 Canonical
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +#include <string.h>
> +#include <signal.h>
> +#include <setjmp.h>
> +
> +#include "fwts.h"
> +
> +static sigjmp_buf jmpbuf;
> +
> +/*
> + *  If we hit a SIGSEGV then the port read
> + *  failed and we longjmp back and return
> + *  FWTS_ERROR
> + */
> +static void segv_handler(int dummy)
> +{
> +	FWTS_UNUSED(dummy);
> +
> +	signal(SIGSEGV, SIG_DFL);
> +	siglongjmp(jmpbuf, 1);
> +}
> +
> +/*
> + *  fwts_safe_memcpy()
> + *	memcpy that catches segfaults. to be used when
> + *	attempting to read BIOS tables from memory which
> + *	may segfault if the src address is corrupt
> + */
> +int fwts_safe_memcpy(void *dst, const void *src, const size_t n)
> +{
> +	if (sigsetjmp(jmpbuf, 1) != 0)
> +		return FWTS_ERROR;
> +	
> +	signal(SIGSEGV, segv_handler);
> +	memcpy(dst, src, n);
> +	signal(SIGSEGV, SIG_DFL);
> +
> +	return FWTS_OK;
> +}
> +
> +int fwts_safe_memread(const void *src, const size_t n)
> +{
> +	unsigned char buf[n];
> +	
> +	return fwts_safe_memcpy(buf, src, n);
> +}

Acked-by: Alex Hung <alex.hung at canonical.com>



More information about the fwts-devel mailing list