The following reply was made to PR kern/148546; it has been noted by GNATS.
From: John Baldwin <j...@freebsd.org>
To: bug-follo...@freebsd.org
Cc: spencer_min...@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
_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
