Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 8 Jul 2012 13:54:40 +0200 (CEST)
From:      Dan Lukes <dan@obluda.cz>
To:        FreeBSD-gnats-submit@FreeBSD.org
Cc:        freebsd-acpi@FreeBSD.org, Hans-Joerg_Hoexer@genua.de
Subject:   bin/169707: [ patch ] improper handling of ACPI TCPA table, acpidump abend imminent
Message-ID:  <201207081154.q68BseNo002031@nb.obluda.cz>
Resent-Message-ID: <201207081200.q68C0OET081372@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help

>Number:         169707
>Category:       bin
>Synopsis:       [ patch ] improper handling of ACPI TCPA table, acpidump abend imminent
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sun Jul 08 12:00:24 UTC 2012
>Closed-Date:
>Last-Modified:
>Originator:     Dan Lukes
>Release:        FreeBSD 9.0 i386
>Organization:
Obludarium
>Environment:
System: FreeBSD 9.0
src/usr.sbin/acpi/acpidump/acpi.c,v 1.42.2.1.2.1

but apply for all revisions past 1.38 (e.g. all RELENG_9 and HEAD)

>Description:
	TCG ACPI (TPCA) support added as SVN rev 211196

1. event->event_type and event->event_size are big-endian (see TPCA PC Specific Specification, paragraph 7.2.2.2). Current code use them directly. It cause misinterpretation of values and may cause abend.

2. 'if (vaddr + event->event_size >= vend )' test is insufficient because:

2a) event->event_size is declared signed and may be negative (especialy when big-endian value used without proper conversion)
2b) vaddr+event->event_size may overflow / wrap around even in the case the event_size is positive

in both cases, memory outside of <vaddr,vend> range may be referenced. Abend is imminent.

>How-To-Repeat:
Dump non-empty TCPA table. It will print events incorrectly, may abend.

>Fix:

1. use ntohl() to convert event->event_size and event->event_type before use
2. test vaddr + eventdatasize for wraparound/underflow case also 

--- usr.sbin/acpi/acpidump/acpi.c.old	2012-07-08 13:14:21.000000000 +0200
+++ usr.sbin/acpi/acpidump/acpi.c	2012-07-08 13:06:46.000000000 +0200
@@ -40,6 +40,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <netinet/in.h>
 
 #include "acpidump.h"
 
@@ -538,10 +539,15 @@
 {
 	struct TCPApc_event *pc_event;
 	char *eventname = NULL;
+	unsigned int eventid, eventdatasize;
+
+	// EventID and EventDataSize are big endian (TPCA PC Specific Specification [7.2.2.2])
+	eventid = ntohl(event->event_type);
+	eventdatasize = ntohl(event->event_size);
 
 	pc_event = (struct TCPApc_event *)(event + 1);
 
-	switch(event->event_type) {
+	switch(eventid) {
 	case PREBOOT:
 	case POST_CODE:
 	case UNUSED:
@@ -559,12 +565,12 @@
 	case NONHOST_CONFIG:
 	case NONHOST_INFO:
 		asprintf(&eventname, "%s",
-		    tcpa_event_type_strings[event->event_type]);
+		    tcpa_event_type_strings[eventid]);
 		break;
 
 	case ACTION:
-		eventname = calloc(event->event_size + 1, sizeof(char));
-		memcpy(eventname, pc_event, event->event_size);
+		eventname = calloc(eventdatasize + 1, sizeof(char));
+		memcpy(eventname, pc_event, eventdatasize);
 		break;
 
 	case EVENT_TAG:
@@ -593,7 +599,7 @@
 		break;
 
 	default:
-		asprintf(&eventname, "<unknown 0x%02x>", event->event_type);
+		asprintf(&eventname, "<unknown 0x%02x>", eventid);
 		break;
 	}
 
@@ -659,20 +665,27 @@
 	vend = vaddr + len;
 
 	while (vaddr != NULL) {
+		unsigned int eventid, eventdatasize;
+		
 		if (vaddr + sizeof(struct TCPAevent) >= vend)
 			break;
 		event = (struct TCPAevent *)(void *)vaddr;
-		if (vaddr + event->event_size >= vend)
+		// EventID and EventDataSize are big endian (TPCA PC Specific Specification [7.2.2.2])
+		eventid = ntohl(event->event_type);
+		eventdatasize = ntohl(event->event_size);
+		if (vaddr + eventdatasize >= vend )
+			break;
+		if (vaddr + eventdatasize < vaddr )
 			break;
-		if (event->event_type == 0 && event->event_size == 0)
+		if (eventid == 0 && eventdatasize == 0)
 			break;
 #if 0
 		{
 		unsigned int i, j, k;
 
-		printf("\n\tsize %d\n\t\t%p ", event->event_size, vaddr);
+		printf("\n\tsize %u\n\t\t%p ", eventdatasize, vaddr);
 		for (j = 0, i = 0; i <
-		    sizeof(struct TCPAevent) + event->event_size; i++) {
+		    sizeof(struct TCPAevent) + eventdatasize; i++) {
 			printf("%02x ", vaddr[i]);
 			if ((i+1) % 8 == 0) {
 				for (k = 0; k < 8; k++)
@@ -686,7 +699,7 @@
 #endif
 		acpi_print_tcpa(event);
 
-		vaddr += sizeof(struct TCPAevent) + event->event_size;
+		vaddr += sizeof(struct TCPAevent) + eventdatasize;
 	}
 
 	printf(END_COMMENT);
--- patch ends here ---

>Release-Note:
>Audit-Trail:
>Unformatted:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201207081154.q68BseNo002031>