From owner-freebsd-bugs@FreeBSD.ORG Tue Jul 13 19:50:03 2010 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D1DFE1065679 for ; Tue, 13 Jul 2010 19:50:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id A4C058FC25 for ; Tue, 13 Jul 2010 19:50:03 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id o6DJo3tS075773 for ; Tue, 13 Jul 2010 19:50:03 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id o6DJo3Rj075772; Tue, 13 Jul 2010 19:50:03 GMT (envelope-from gnats) Date: Tue, 13 Jul 2010 19:50:03 GMT Message-Id: <201007131950.o6DJo3Rj075772@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: John Baldwin Cc: Subject: Re: kern/148546: [ipmi] Buffer overrun in the impi driver while processing smbios date X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: John Baldwin List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jul 2010 19:50:03 -0000 The following reply was made to PR kern/148546; it has been noted by GNATS. From: John Baldwin To: bug-followup@freebsd.org Cc: spencer_minear@mcafee.com Subject: Re: kern/148546: [ipmi] Buffer overrun in the impi driver while processing smbios date Date: Tue, 13 Jul 2010 15:48:10 -0400 On Tuesday, July 13, 2010 3:06:02 pm John Baldwin wrote: > Hmm, the smbios table parser in ipmi_smbios.c is a bit broken. :( I > think it was derived from a more generic parser. At some point it might > be useful to write a more generic smbios table parser that this code > could use, but the simplest fix might be to just simplify this code to > be more IPMI specific. For example, the IPMI table entry doesn't use > the strings at all, so the table of strings could just be dropped. We > could also remove the dispatch table and instead check the table entry > type in the the smbios_t38_proc_info() function. This is more like what > other places in the kernel do when walking tables e.g. the MADT or MP Table. Here is a possible implementation of this. I have compiled it but have not yet run tested it: Index: ipmi_smbios.c =================================================================== --- ipmi_smbios.c (revision 209948) +++ ipmi_smbios.c (working copy) @@ -52,7 +52,7 @@ #define pmap_unmapbios pmap_unmapdev #endif -struct smbios_table_entry { +struct smbios_table { uint8_t anchor_string[4]; uint8_t checksum; uint8_t length; @@ -108,27 +108,30 @@ #define SMBIOS_LEN 4 #define SMBIOS_SIG "_SM_" -typedef void (*dispatchproc_t)(uint8_t *p, char **table, - struct ipmi_get_info *info); +typedef void (*smbios_callback_t)(struct structure_header *, void *); static struct ipmi_get_info ipmi_info; static int ipmi_probed; static struct mtx ipmi_info_mtx; MTX_SYSINIT(ipmi_info, &ipmi_info_mtx, "ipmi info", MTX_DEF); -static char *get_strings(char *, char **); static void ipmi_smbios_probe(struct ipmi_get_info *); -static int smbios_cksum (struct smbios_table_entry *); -static void smbios_run_table(uint8_t *, int, dispatchproc_t *, - struct ipmi_get_info *); -static void smbios_t38_proc_info(uint8_t *, char **, - struct ipmi_get_info *); +static int smbios_cksum(struct smbios_table *); +static void smbios_walk_table(uint8_t *, int, smbios_callback_t, + void *); +static void smbios_ipmi_info(struct structure_header *, void *); static void -smbios_t38_proc_info(uint8_t *p, char **table, struct ipmi_get_info *info) +smbios_ipmi_info(struct structure_header *h, void *arg) { - struct ipmi_entry *s = (struct ipmi_entry *)p; + struct ipmi_get_info *info; + struct ipmi_entry *s; + if (h->type != 38 || h->length < + offsetof(struct ipmi_entry, interrupt_number)) + return; + s = (struct ipmi_entry *)h; + info = arg; bzero(info, sizeof(struct ipmi_get_info)); switch (s->interface_type) { case KCS_MODE: @@ -172,44 +175,28 @@ info->iface_type = s->interface_type; } -static char * -get_strings(char *p, char **table) -{ - /* Scan for strings, stoping at a single null byte */ - while (*p != 0) { - *table++ = p; - p += strlen(p) + 1; - } - *table = 0; - - /* Skip past terminating null byte */ - return (p + 1); -} - - static void -smbios_run_table(uint8_t *p, int entries, dispatchproc_t *dispatchstatus, - struct ipmi_get_info *info) +smbios_walk_table(uint8_t *p, int entries, smbios_callback_t cb, void *arg) { struct structure_header *s; - char *table[20]; - uint8_t *nextp; - while(entries--) { - s = (struct structure_header *) p; - nextp = get_strings(p + s->length, table); + while (entries--) { + s = (struct structure_header *)p; + cb(s, arg); /* - * No strings still has a double-null at the end, - * skip over it + * Look for a double-nul after the end of the + * formatted area of this structure. */ - if (table[0] == 0) - nextp++; + p += s->length; + while (p[0] != 0 && p[1] != 0) + p++; - if (dispatchstatus[*p]) { - (dispatchstatus[*p])(p, table, info); - } - p = nextp; + /* + * Skip over the double-nul to the start of the next + * structure. + */ + p += 2; } } @@ -221,8 +208,7 @@ static void ipmi_smbios_probe(struct ipmi_get_info *info) { - dispatchproc_t dispatch_smbios_ipmi[256]; - struct smbios_table_entry *header; + struct smbios_table *header; void *table; u_int32_t addr; @@ -239,9 +225,9 @@ * length and then map it a second time with the actual length so * we can verify the checksum. */ - header = pmap_mapbios(addr, sizeof(struct smbios_table_entry)); + header = pmap_mapbios(addr, sizeof(struct smbios_table)); table = pmap_mapbios(addr, header->length); - pmap_unmapbios((vm_offset_t)header, sizeof(struct smbios_table_entry)); + pmap_unmapbios((vm_offset_t)header, sizeof(struct smbios_table)); header = table; if (smbios_cksum(header) != 0) { pmap_unmapbios((vm_offset_t)header, header->length); @@ -251,9 +237,7 @@ /* Now map the actual table and walk it looking for an IPMI entry. */ table = pmap_mapbios(header->structure_table_address, header->structure_table_length); - bzero((void *)dispatch_smbios_ipmi, sizeof(dispatch_smbios_ipmi)); - dispatch_smbios_ipmi[38] = (void *)smbios_t38_proc_info; - smbios_run_table(table, header->number_structures, dispatch_smbios_ipmi, + smbios_walk_table(table, header->number_structures, smbios_ipmi_info, info); /* Unmap everything. */ @@ -298,7 +282,7 @@ } static int -smbios_cksum (struct smbios_table_entry *e) +smbios_cksum(struct smbios_table *e) { u_int8_t *ptr; u_int8_t cksum; -- John Baldwin