Patchwork [06/11] lib: fwts_acpi_tables: ensure we don't overflow a table name when given bad input

login
register
mail settings
Submitter Colin King
Date April 11, 2012, 11:50 p.m.
Message ID <1334188256-26566-7-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/151941/
State Accepted
Headers show

Comments

Colin King - April 11, 2012, 11:50 p.m.
From: Colin Ian King <colin.king@canonical.com>

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_acpi_tables.c |   30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)
Keng-Yu Lin - April 12, 2012, 6:17 a.m.
On Thu, Apr 12, 2012 at 7:50 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_acpi_tables.c |   30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index 0a90d37..f09e94c 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -19,6 +19,7 @@
>
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <stddef.h>
>  #include <stdbool.h>
>  #include <string.h>
>  #include <unistd.h>
> @@ -308,25 +309,40 @@ static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
>  {
>        uint32_t offset;
>        uint8_t  data[16];
> -       char buffer[80];
> +       char buffer[128];
>        uint8_t *table = NULL;
>        char *ptr = buffer;
>        size_t len = 0;
>        unsigned long long table_addr;
> +       ptrdiff_t name_len;
>
>        *size = 0;
>
>        if (fgets(buffer, sizeof(buffer), fp) == NULL)
>                return NULL;
>
> -       for (ptr = buffer; *ptr && *ptr != '@'; ptr++)
> -               ;
> -
> -       if ((*ptr != '@') || ((ptr - buffer) < 5))
> -               return NULL; /* Bad name? */
> +       /*
> +        * Parse tablename followed by address, e.g.
> +        *   DSTD @ 0xbfa02344
> +        *   SSDT4 @ 0xbfa0f230
> +        */
> +       ptr = strstr(buffer, "@ 0x");
> +       if (ptr == NULL)
> +               return NULL; /* Can't find table name */
> +
> +       name_len = ptr - buffer;
> +       /*
> +        * We should have no more than the table name (4..5 chars)
> +        * plus a space left between the start of the buffer and
> +        * the @ sign.  If we have more then something is wrong with
> +        * the data. So just ignore this garbage as we don't want to
> +        * overflow the name on the following strcpy()
> +        */
> +       if ((name_len > 6) || (name_len < 5))
> +               return NULL; /* Name way too long or too short */
>
>        if (sscanf(ptr, "@ 0x%Lx\n", &table_addr) < 1)
> -               return NULL;
> +               return NULL; /* Can't parse address */
>
>        *(ptr-1) = '\0';
>        strcpy(name, buffer);
> --
> 1.7.9.5
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Alex Hung - April 12, 2012, 7:51 a.m.
On 04/12/2012 07:50 AM, Colin King wrote:
> From: Colin Ian King<colin.king@canonical.com>
>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---

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

Patch

diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
index 0a90d37..f09e94c 100644
--- a/src/lib/src/fwts_acpi_tables.c
+++ b/src/lib/src/fwts_acpi_tables.c
@@ -19,6 +19,7 @@ 
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stddef.h>
 #include <stdbool.h>
 #include <string.h>
 #include <unistd.h>
@@ -308,25 +309,40 @@  static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
 {
 	uint32_t offset;
 	uint8_t  data[16];
-	char buffer[80];
+	char buffer[128];
 	uint8_t *table = NULL;
 	char *ptr = buffer;
 	size_t len = 0;
 	unsigned long long table_addr;
+	ptrdiff_t name_len;
 
 	*size = 0;
 
 	if (fgets(buffer, sizeof(buffer), fp) == NULL)
 		return NULL;
 
-	for (ptr = buffer; *ptr && *ptr != '@'; ptr++)
-		;
-
-	if ((*ptr != '@') || ((ptr - buffer) < 5))
-		return NULL; /* Bad name? */
+	/*
+	 * Parse tablename followed by address, e.g.
+	 *   DSTD @ 0xbfa02344 
+	 *   SSDT4 @ 0xbfa0f230 
+	 */
+	ptr = strstr(buffer, "@ 0x");
+	if (ptr == NULL)
+		return NULL; /* Can't find table name */
+
+	name_len = ptr - buffer;
+	/*
+	 * We should have no more than the table name (4..5 chars)
+	 * plus a space left between the start of the buffer and
+	 * the @ sign.  If we have more then something is wrong with
+	 * the data. So just ignore this garbage as we don't want to
+	 * overflow the name on the following strcpy()
+	 */
+	if ((name_len > 6) || (name_len < 5))
+		return NULL; /* Name way too long or too short */
 
 	if (sscanf(ptr, "@ 0x%Lx\n", &table_addr) < 1)
-		return NULL;
+		return NULL; /* Can't parse address */
 	
 	*(ptr-1) = '\0';
 	strcpy(name, buffer);