From: Konrad Rzeszutek <konradr@redhat.com> Date: Wed, 20 Feb 2008 13:20:22 -0500 Subject: [firmware] ibft_iscsi: prevent misconfigured iBFTs Message-id: 20080220182022.GA3331@mars.boston.redhat.com O-Subject: Re: [RHEL5 PATCH] RHBZ#430297 - ibft_iscsi when used with Intel NIC which have iBFT misconfigured crashes. Bugzilla: 430297 > > > On Fri, Jan 25, 2008 at 05:11:52PM -0500, Konrad Rzeszutek wrote: > > > > RHBZ#: > > > > ------ > > > > https://bugzilla.redhat.com/show_bug.cgi?id=430297 > > > > > > > > Description: > > > > ------------ > > > > The Intel NICs which support iSCSI can write a completely bogus iBFT structure. > > > > The iscsi_ibft doesn't handle that gracefully and this can result in a panic. > > > > The work-around is to configure the Intel NIC properly - which might not be > > > > obvious to causal user or this patch. > > > > > > > > Attached ia a patch to RHEL5 which makes the module more robust. > > > > > > > > Upstream Status: > > > > ---------------- > > > > http://lkml.org/lkml/2008/1/25/549 > > > Acked-by: Mike Christie <mchristi@redhat.com> diff --git a/arch/i386/kernel/setup.c b/arch/i386/kernel/setup.c index 4f74495..34b515d 100644 --- a/arch/i386/kernel/setup.c +++ b/arch/i386/kernel/setup.c @@ -721,20 +721,6 @@ static inline void copy_edd(void) } #endif -#if defined(CONFIG_ISCSI_IBFT_FIND) -static void __init reserve_ibft_region(void) -{ - unsigned int ibft_len; - - ibft_len = find_ibft(); - if (ibft_len) - reserve_bootmem((unsigned int)ibft_phys, - PAGE_ALIGN(ibft_len)); -} -#else -static void __init reserve_ibft_region(void) { } -#endif - static void __init parse_cmdline_early (char ** cmdline_p) { char c = ' ', *to = command_line, *from = saved_command_line; diff --git a/arch/x86_64/kernel/setup.c b/arch/x86_64/kernel/setup.c index 3278347..19ca938 100644 --- a/arch/x86_64/kernel/setup.c +++ b/arch/x86_64/kernel/setup.c @@ -494,20 +494,6 @@ static inline void copy_edd(void) } #endif -#if defined(CONFIG_ISCSI_IBFT_FIND) -static void __init reserve_ibft_region(void) -{ - unsigned int ibft_len; - - ibft_len = find_ibft(); - if (ibft_len) - reserve_bootmem_generic((unsigned long)ibft_phys, - PAGE_ALIGN(ibft_len)); -} -#else -static void __init reserve_ibft_region(void) { } -#endif - #define EBDA_ADDR_POINTER 0x40E unsigned __initdata ebda_addr; diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 9f1b54e..9a0acfa 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -86,6 +86,7 @@ config DCDBAS config ISCSI_IBFT_FIND bool "iSCSI Boot Firmware Table Attributes" + depends on X86 default n help This option enables the kernel to find the region of memory diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c index bbd41f1..4c76064 100644 --- a/drivers/firmware/iscsi_ibft.c +++ b/drivers/firmware/iscsi_ibft.c @@ -1,7 +1,7 @@ /* * Copyright 2007 Red Hat, Inc. * by Peter Jones <pjones@redhat.com> - * Copyright 2007 IBM, Inc. + * Copyright 2008 IBM, Inc. * by Konrad Rzeszutek <konradr@linux.vnet.ibm.com> * * This code exposes the iSCSI Boot Format Table to userland via sysfs. @@ -14,6 +14,48 @@ * 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. + * + * Changelog: + * + * 11 Feb 2008 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Converted to using ibft_addr. (v0.4.8) + * + * 8 Feb 2008 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Combined two functions in one: reserve_ibft_region. (v0.4.7) + * + * 30 Jan 2008 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added logic to handle IPv6 addresses. (v0.4.6) + * + * 25 Jan 2008 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added logic to handle badly not-to-spec iBFT. (v0.4.5) + * + * 4 Jan 2008 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added __init to function declarations. (v0.4.4) + * + * 21 Dec 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Updated kobject registration, combined unregister functions in one + * and code and style cleanup. (v0.4.3) + * + * 5 Dec 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added end-markers to enums and re-organized kobject registration. (v0.4.2) + * + * 4 Dec 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Created 'device' sysfs link to the NIC and style cleanup. (v0.4.1) + * + * 28 Nov 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added sysfs-ibft documentation, moved 'find_ibft' function to + * in its own file and added text attributes for every struct field. (v0.4) + * + * 21 Nov 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added text attributes emulating OpenFirmware /proc/device-tree naming. + * Removed binary /sysfs interface (v0.3) + * + * 29 Aug 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * Added functionality in setup.c to reserve iBFT region. (v0.2) + * + * 27 Aug 2007 - Konrad Rzeszutek <konradr@linux.vnet.ibm.com> + * First version exposing iBFT data via a binary /sysfs. (v0.1) + * */ @@ -31,8 +73,8 @@ #include <linux/string.h> #include <linux/types.h> -#define IBFT_ISCSI_VERSION "0.4.4-rhel5" -#define IBFT_ISCSI_DATE "2008-Jan-9" +#define IBFT_ISCSI_VERSION "0.4.8-rhel5" +#define IBFT_ISCSI_DATE "2008-Feb-11" MODULE_AUTHOR("Peter Jones <pjones@redhat.com> and \ Konrad Rzeszutek <konradr@us.ibm.com>"); @@ -40,17 +82,6 @@ MODULE_DESCRIPTION("sysfs interface to BIOS iBFT information"); MODULE_LICENSE("GPL"); MODULE_VERSION(IBFT_ISCSI_VERSION); - -struct ibft_table_header { - char signature[4]; - u32 length; - u8 revision; - u8 checksum; - char oem_id[6]; - char oem_table_id[8]; - char reserved[24]; -} __attribute__((__packed__)); - struct ibft_hdr { u8 id; u8 version; @@ -119,17 +150,20 @@ struct ibft_tgt { * */ enum ibft_id { - id_reserved = 0, /* Should never show up */ - id_control = 1, /* Ditto */ + id_reserved = 0, /* We don't support. */ + id_control = 1, /* Should show up only once and is not exported. */ id_initiator = 2, id_nic = 3, id_target = 4, - id_extensions, /* We don't support. */ + id_extensions = 5, /* We don't support. */ + id_end_marker, }; +/* + * We do not support the other types, hence the NULL. + */ static const char *ibft_id_names[] = - {"reserved", "control", "initiator", "ethernet%d", - "target%d", "extension%d", NULL}; + {NULL, NULL, "initiator", "ethernet%d", "target%d", NULL, NULL}; /* * The text attributes names for each of the kobjects. @@ -229,23 +263,32 @@ static LIST_HEAD(ibft_attr_list); static LIST_HEAD(ibft_kobject_list); static const char nulls[16]; -static struct ibft_table_header *ibft_device; /* * Helper functions to parse data properly. */ static ssize_t sprintf_ipaddr(char *buf, u8 *ip) { + char *str = buf; + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 && ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 && ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) { /* * IPV4 */ - return sprintf(buf, "%d.%d.%d.%d\n", ip[12], + str += sprintf(buf, NIPQUAD_FMT, ip[12], ip[13], ip[14], ip[15]); - } else - return 0; + } else { + /* + * IPv6 + */ + str += sprintf(str, NIP6_FMT, ntohs(ip[0]), ntohs(ip[1]), + ntohs(ip[2]), ntohs(ip[3]), ntohs(ip[4]), + ntohs(ip[5]), ntohs(ip[6]), ntohs(ip[7])); + } + str += sprintf(str, "\n"); + return str - buf; } static ssize_t sprintf_string(char *str, int len, char *buf) @@ -256,18 +299,21 @@ static ssize_t sprintf_string(char *str, int len, char *buf) /* * Helper function to verify the IBFT header. */ -static int ibft_verify_hdr(struct ibft_hdr *hdr, int id, int length) +static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int length) { -#define IBFT_VERIFY_HDR_FIELD(val, name) \ - if (hdr->val != val) { \ - printk(KERN_ERR \ - "error, in IBFT structure (%s) expected %d but" \ - " found %d\n", \ - name, val, hdr->val); \ - return -ENODEV; \ + if (hdr->id != id) { + printk(KERN_ERR "iBFT error: The field header.id is " \ + "expected %d but found %d!\n", \ + id, hdr->id); + return -ENODEV; + } + if (hdr->length != length) { + printk(KERN_ERR "iBFT error: The field header.length is " \ + "expected %d but found %d!\n", \ + length, hdr->length); + return -ENODEV; } - IBFT_VERIFY_HDR_FIELD(id, "ID"); - IBFT_VERIFY_HDR_FIELD(length, "Length"); + return 0; } @@ -486,32 +532,31 @@ static struct kobj_type ibft_ktype = { static decl_subsys(ibft, &ibft_ktype, NULL); -static int __init ibft_alloc_device(void *idev) +static int __init ibft_check_device(void) { int len; - struct ibft_table_header *hdr; - - if (!idev) - return -ENOENT; + u8 *pos; + u8 csum = 0; - hdr = (struct ibft_table_header *)phys_to_virt((unsigned long) - ibft_phys); - len = hdr->length; + len = ibft_addr->length; - ibft_device = kmalloc(len, GFP_KERNEL); - if (!ibft_device) - return -ENOMEM; + /* Sanity checking of iBFT. */ + if (ibft_addr->revision != 1) { + printk(KERN_ERR "iBFT module supports only revision 1, " \ + "while this is %d.\n", ibft_addr->revision); + return 1; + } + for (pos = (u8 *)ibft_addr; pos < (u8 *)ibft_addr + len; pos++) + csum += *pos; - memcpy(ibft_device, hdr, len); + if (csum) { + printk(KERN_ERR "iBFT has incorrect checksum (0x%x)!\n", csum); + return 1; + } return 0; } -static void ibft_free_device(struct ibft_table_header *hdr) -{ - kfree(hdr); -} - /* * Helper function for ibft_register_kobjects. */ @@ -533,25 +578,33 @@ static int __init ibft_create_kobject(struct ibft_table_header *header, switch (hdr->id) { case id_initiator: - rc = ibft_verify_hdr(hdr, id_initiator, + rc = ibft_verify_hdr("initiator", hdr, id_initiator, sizeof(*ibft_kobj->initiator)); break; case id_nic: - rc = ibft_verify_hdr(hdr, id_nic, + rc = ibft_verify_hdr("ethernet", hdr, id_nic, sizeof(*ibft_kobj->nic)); break; case id_target: - rc = ibft_verify_hdr(hdr, id_target, + rc = ibft_verify_hdr("target", hdr, id_target, sizeof(*ibft_kobj->tgt)); break; + case id_reserved: + case id_control: + case id_extensions: + /* Fields which we don't support. Ignore them */ + rc = 1; + break; default: - /* Extension field which we don't support. Ignore it */ + printk(KERN_ERR "iBFT has unknown structure type (%d). " \ + "Report this bug to your vendor!\n", hdr->id); + rc = 1; break; } if (rc) { kfree(ibft_kobj); - goto out; + goto out_invalid_struct; } /* kobject fief. We will let the reference counter do the job @@ -592,6 +645,9 @@ kobject_do_put: kobject_put(&ibft_kobj->kobj); out: return rc; +out_invalid_struct: + /* Unsupported structs are skipped. */ + return 0; } /* @@ -605,13 +661,21 @@ static int __init ibft_register_kobjects(struct ibft_table_header *header, void *ptr, *end; int rc = 0; u16 offset; + u16 eot_offset; control = (void *)header + sizeof(*header); end = (void *)control + control->hdr.length; + eot_offset = (void *)header + header->length - + (void *)control - sizeof(*header); + rc = ibft_verify_hdr("control", (struct ibft_hdr *)control, id_control, + sizeof(*control)); + rc |= control->hdr.index != 0; + if (rc) + return rc; for (ptr = &control->initiator_off; ptr < end; ptr += sizeof(u16)) { offset = *(u16 *)ptr; - if (offset && offset < header->length) { + if (offset && offset < header->length && offset < eot_offset) { rc = ibft_create_kobject(header, (void *)header + offset, list); @@ -863,22 +927,20 @@ static int __init ibft_init(void) { int rc = 0; - if (!ibft_phys) - find_ibft(); - rc = firmware_register(&ibft_subsys); if (rc) return rc; - if (ibft_phys) { - printk(KERN_INFO "iBFT detected at 0x%p.\n", ibft_phys); + if (ibft_addr) { + printk(KERN_INFO "iBFT detected at 0x%lx.\n", + virt_to_phys((void *)ibft_addr)); - rc = ibft_alloc_device(ibft_phys); + rc = ibft_check_device(); if (rc) goto out_firmware_unregister; /* Scan the IBFT for data and register kobjects. */ - rc = ibft_register_kobjects(ibft_device, &ibft_kobject_list); + rc = ibft_register_kobjects(ibft_addr, &ibft_kobject_list); if (rc) goto out_free; @@ -894,7 +956,6 @@ static int __init ibft_init(void) out_free: ibft_unregister(&ibft_attr_list, &ibft_kobject_list); - ibft_free_device(ibft_device); out_firmware_unregister: firmware_unregister(&ibft_subsys); return rc; @@ -903,7 +964,6 @@ out_firmware_unregister: static void __exit ibft_exit(void) { ibft_unregister(&ibft_attr_list, &ibft_kobject_list); - ibft_free_device(ibft_device); firmware_unregister(&ibft_subsys); } diff --git a/drivers/firmware/iscsi_ibft_find.c b/drivers/firmware/iscsi_ibft_find.c index d40cc25..8ef433a 100644 --- a/drivers/firmware/iscsi_ibft_find.c +++ b/drivers/firmware/iscsi_ibft_find.c @@ -16,6 +16,7 @@ * GNU General Public License for more details. */ +#include <linux/bootmem.h> #include <linux/blkdev.h> #include <linux/ctype.h> #include <linux/device.h> @@ -29,12 +30,13 @@ #include <linux/string.h> #include <linux/types.h> +#include <asm/mmzone.h> /* * Physical location of iSCSI Boot Format Table. */ -void *ibft_phys; -EXPORT_SYMBOL(ibft_phys); +struct ibft_table_header *ibft_addr; +EXPORT_SYMBOL(ibft_addr); #define IBFT_SIGN "iBFT" #define IBFT_SIGN_LEN 4 @@ -45,16 +47,18 @@ EXPORT_SYMBOL(ibft_phys); /* - * Routine used to find the iSCSI Boot Format Table. the physical - * location is set in the ibft_phys variable. The return value is - * the size of IBFT. + * Routine used to find the iSCSI Boot Format Table. the logical + * kernel address is set in the ibft_addr variable. */ -ssize_t find_ibft(void) +void __init reserve_ibft_region(void) { unsigned long pos; + unsigned int len = 0; + void *virt; + + ibft_addr = 0; for (pos = IBFT_START; pos < IBFT_END; pos += 16) { - void *virt; /* The table can't be inside the VGA BIOS reserved space, * so skip that area */ if (pos == VGA_MEM) @@ -63,15 +67,16 @@ ssize_t find_ibft(void) if (memcmp(virt, IBFT_SIGN, IBFT_SIGN_LEN) == 0) { unsigned long *addr = (unsigned long *)phys_to_virt(pos + 4); - unsigned int len = *addr; + len = *addr; /* if the length of the table extends past 1M, * the table cannot be valid. */ if (pos + len <= (IBFT_END-1)) { - ibft_phys = (void *)pos; - return len; + ibft_addr = (struct ibft_table_header *)virt; + break; } } } - return 0; + if (ibft_addr) + reserve_bootmem(pos, PAGE_ALIGN(len)); } -EXPORT_SYMBOL(find_ibft); +EXPORT_SYMBOL(reserve_ibft_region); diff --git a/include/linux/iscsi_ibft.h b/include/linux/iscsi_ibft.h index d36ca16..6b12659 100644 --- a/include/linux/iscsi_ibft.h +++ b/include/linux/iscsi_ibft.h @@ -19,20 +19,30 @@ #ifndef ISCSI_IBFT_H #define ISCSI_IBFT_H -#if defined(CONFIG_ISCSI_IBFT_FIND) +struct ibft_table_header { + char signature[4]; + u32 length; + u8 revision; + u8 checksum; + char oem_id[6]; + char oem_table_id[8]; + char reserved[24]; +} __attribute__((__packed__)); /* - * Physical location of iSCSI Boot Format Table. + * Logical location of iSCSI Boot Format Table. + * If the value is NULL there is no iBFT on the machine. */ -extern void *ibft_phys; +extern struct ibft_table_header *ibft_addr; /* - * Routine used to find the iSCSI Boot Format Table. The physical - * location is set in the ibft_phys variable. The return value is the - * size of IBFT. + * Routine used to find and reserve the iSCSI Boot Format Table. The + * mapped address is set in the ibft_addr variable. */ -extern ssize_t find_ibft(void); - +#ifdef CONFIG_ISCSI_IBFT_FIND +extern void __init reserve_ibft_region(void); +#else +static inline void reserve_ibft_region(void) { } #endif #endif /* ISCSI_IBFT_H */