* [PATCH 1/3] firewire: core: fix "giving up on config rom" with Panasonic AG-DV2500 @ 2010-02-18 0:50 Stefan Richter 2010-02-18 0:52 ` [PATCH 2/3] firewire: core: don't fail device creation in case of too large config ROM blocks Stefan Richter 0 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2010-02-18 0:50 UTC (permalink / raw) To: linux1394-devel; +Cc: linux-kernel The Panasonic AG-DV2500 tape deck contains an invalid entry in its configuration ROM root directory: A leaf pointer with the undefined key ID 0 and an offset that points way out of the standard config ROM area. This caused firewire-core to dismiss the device with the generic log message "giving up on config rom for node id...", after which it was of course impossible to access the tape deck with dvgrab or any other program. https://bugzilla.redhat.com/show_bug.cgi?id=449252#c29 The fix is to simply ignore this invalid ROM entry and proceed to read the valid rest of the ROM. There is a catch though: When the kernel later iterates over the ROM, it would be nasty having to check again for such too large ROM offsets. Therefore we manipulate the defective or unsupported ROM entry to become a harmless immediate entry that won't have any side effects later (an entry with the value 0x00000000). Reported-by: George Chriss Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-device.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) Index: linux-2.6.33-rc8/drivers/firewire/core-device.c =================================================================== --- linux-2.6.33-rc8.orig/drivers/firewire/core-device.c +++ linux-2.6.33-rc8/drivers/firewire/core-device.c @@ -18,6 +18,7 @@ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. */ +#include <linux/bug.h> #include <linux/ctype.h> #include <linux/delay.h> #include <linux/device.h> @@ -580,11 +581,7 @@ static int read_bus_info_block(struct fw */ key = stack[--sp]; i = key & 0xffffff; - if (i >= READ_BIB_ROM_SIZE) - /* - * The reference points outside the standard - * config rom area, something's fishy. - */ + if (WARN_ON(i >= READ_BIB_ROM_SIZE)) goto out; /* Read header quadlet for the block to get the length. */ @@ -606,14 +603,29 @@ static int read_bus_info_block(struct fw * block, check the entries as we read them to see if * it references another block, and push it in that case. */ - while (i < end) { + for (; i < end; i++) { if (read_rom(device, generation, i, &rom[i]) != RCODE_COMPLETE) goto out; - if ((key >> 30) == 3 && (rom[i] >> 30) > 1 && - sp < READ_BIB_STACK_SIZE) - stack[sp++] = i + rom[i]; - i++; + + if ((key >> 30) != 3 || (rom[i] >> 30) < 2 || + sp >= READ_BIB_STACK_SIZE) + continue; + /* + * Offset points outside the ROM. May be a firmware + * bug or an Extended ROM entry (IEEE 1212-2001 clause + * 7.7.18). Simply overwrite this pointer here by a + * fake immediate entry so that later iterators over + * the ROM don't have to check offsets all the time. + */ + if (i + (rom[i] & 0xffffff) >= READ_BIB_ROM_SIZE) { + fw_error("skipped unsupported ROM entry %x at %llx\n", + rom[i], + i * 4 | CSR_REGISTER_BASE | CSR_CONFIG_ROM); + rom[i] = 0; + continue; + } + stack[sp++] = i + rom[i]; } if (length < i) length = i; -- Stefan Richter -=====-==-=- --=- =--=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/3] firewire: core: don't fail device creation in case of too large config ROM blocks 2010-02-18 0:50 [PATCH 1/3] firewire: core: fix "giving up on config rom" with Panasonic AG-DV2500 Stefan Richter @ 2010-02-18 0:52 ` Stefan Richter 2010-02-18 0:54 ` [PATCH 3/3] firewire: core: increase stack size of config ROM reader Stefan Richter 0 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2010-02-18 0:52 UTC (permalink / raw) To: linux1394-devel; +Cc: linux-kernel It never happened yet, but better safe than sorry: If a device's config ROM contains a block which overlaps the boundary at 0xfffff00007ff, just ignore that one block instead of refusing to add the device representation. That way, upper layers (kernelspace or userspace drivers) might still be able to use the device to some degree. That's better than total inaccessibility of the device. Worse, the core would have logged only a generic "giving up on config rom" message which could only be debugged by feeding a firewire-ohci debug logging session through a config ROM interpreter, IOW would likely remain undiagnosed. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-device.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) Index: linux-2.6.33-rc8/drivers/firewire/core-device.c =================================================================== --- linux-2.6.33-rc8.orig/drivers/firewire/core-device.c +++ linux-2.6.33-rc8/drivers/firewire/core-device.c @@ -588,15 +588,19 @@ static int read_bus_info_block(struct fw if (read_rom(device, generation, i, &rom[i]) != RCODE_COMPLETE) goto out; end = i + (rom[i] >> 16) + 1; - i++; - if (end > READ_BIB_ROM_SIZE) + if (end > READ_BIB_ROM_SIZE) { /* - * This block extends outside standard config - * area (and the array we're reading it - * into). That's broken, so ignore this - * device. + * This block extends outside the config ROM which is + * a firmware bug. Ignore this whole block, i.e. + * simply set a fake block length of 0. */ - goto out; + fw_error("skipped invalid ROM block %x at %llx\n", + rom[i], + i * 4 | CSR_REGISTER_BASE | CSR_CONFIG_ROM); + rom[i] = 0; + end = i; + } + i++; /* * Now read in the block. If this is a directory -- Stefan Richter -=====-==-=- --=- =--=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/3] firewire: core: increase stack size of config ROM reader 2010-02-18 0:52 ` [PATCH 2/3] firewire: core: don't fail device creation in case of too large config ROM blocks Stefan Richter @ 2010-02-18 0:54 ` Stefan Richter 2010-02-19 20:00 ` [PATCH] firewire: core: fix an information leak Stefan Richter 0 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2010-02-18 0:54 UTC (permalink / raw) To: linux1394-devel; +Cc: linux-kernel The stack size of 16 was artificially chosen and may be too small in extreme cases. A device won't be accessible then. Since it doesn't really matter to the slab allocator whether we ask for 1088 bytes or 2048 bytes of scratch memory, just allocate 2048 bytes for the sum of temporary config ROM image and stack, and we will never ever overflow the stack (because there simply can't be more stack items than ROM entries). Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-device.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) Index: linux-2.6.33-rc8/drivers/firewire/core-device.c =================================================================== --- linux-2.6.33-rc8.orig/drivers/firewire/core-device.c +++ linux-2.6.33-rc8/drivers/firewire/core-device.c @@ -493,7 +493,6 @@ static int read_rom(struct fw_device *de } #define READ_BIB_ROM_SIZE 256 -#define READ_BIB_STACK_SIZE 16 /* * Read the bus info block, perform a speed probe, and read all of the rest of @@ -510,7 +509,7 @@ static int read_bus_info_block(struct fw int i, end, length, ret = -1; rom = kmalloc(sizeof(*rom) * READ_BIB_ROM_SIZE + - sizeof(*stack) * READ_BIB_STACK_SIZE, GFP_KERNEL); + sizeof(*stack) * READ_BIB_ROM_SIZE, GFP_KERNEL); if (rom == NULL) return -ENOMEM; @@ -612,8 +611,7 @@ static int read_bus_info_block(struct fw RCODE_COMPLETE) goto out; - if ((key >> 30) != 3 || (rom[i] >> 30) < 2 || - sp >= READ_BIB_STACK_SIZE) + if ((key >> 30) != 3 || (rom[i] >> 30) < 2) continue; /* * Offset points outside the ROM. May be a firmware -- Stefan Richter -=====-==-=- --=- =--=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] firewire: core: fix an information leak 2010-02-18 0:54 ` [PATCH 3/3] firewire: core: increase stack size of config ROM reader Stefan Richter @ 2010-02-19 20:00 ` Stefan Richter 2010-02-19 20:00 ` [PATCH] firewire: core: rename an internal function Stefan Richter 0 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2010-02-19 20:00 UTC (permalink / raw) To: linux1394-devel; +Cc: linux-kernel If a device exposes a sparsely populated configuration ROM, firewire-core's sysfs interface and character device file interface showed random data in the gaps between config ROM blocks. Fix this by zero-initialization of the config ROM reader's scratch buffer. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-device.c | 1 + 1 file changed, 1 insertion(+) Index: b/drivers/firewire/core-device.c =================================================================== --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -514,6 +514,7 @@ static int read_bus_info_block(struct fw return -ENOMEM; stack = &rom[READ_BIB_ROM_SIZE]; + memset(rom, 0, sizeof(*rom) * READ_BIB_ROM_SIZE); device->max_speed = SCODE_100; -- Stefan Richter -=====-==-=- --=- =--== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] firewire: core: rename an internal function 2010-02-19 20:00 ` [PATCH] firewire: core: fix an information leak Stefan Richter @ 2010-02-19 20:00 ` Stefan Richter 0 siblings, 0 replies; 5+ messages in thread From: Stefan Richter @ 2010-02-19 20:00 UTC (permalink / raw) To: linux1394-devel; +Cc: linux-kernel according to what it really does. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-device.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) Index: b/drivers/firewire/core-device.c =================================================================== --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -492,29 +492,29 @@ static int read_rom(struct fw_device *de return rcode; } -#define READ_BIB_ROM_SIZE 256 +#define MAX_CONFIG_ROM_SIZE 256 /* * Read the bus info block, perform a speed probe, and read all of the rest of * the config ROM. We do all this with a cached bus generation. If the bus - * generation changes under us, read_bus_info_block will fail and get retried. + * generation changes under us, read_config_rom will fail and get retried. * It's better to start all over in this case because the node from which we * are reading the ROM may have changed the ROM during the reset. */ -static int read_bus_info_block(struct fw_device *device, int generation) +static int read_config_rom(struct fw_device *device, int generation) { const u32 *old_rom, *new_rom; u32 *rom, *stack; u32 sp, key; int i, end, length, ret = -1; - rom = kmalloc(sizeof(*rom) * READ_BIB_ROM_SIZE + - sizeof(*stack) * READ_BIB_ROM_SIZE, GFP_KERNEL); + rom = kmalloc(sizeof(*rom) * MAX_CONFIG_ROM_SIZE + + sizeof(*stack) * MAX_CONFIG_ROM_SIZE, GFP_KERNEL); if (rom == NULL) return -ENOMEM; - stack = &rom[READ_BIB_ROM_SIZE]; - memset(rom, 0, sizeof(*rom) * READ_BIB_ROM_SIZE); + stack = &rom[MAX_CONFIG_ROM_SIZE]; + memset(rom, 0, sizeof(*rom) * MAX_CONFIG_ROM_SIZE); device->max_speed = SCODE_100; @@ -581,14 +581,14 @@ static int read_bus_info_block(struct fw */ key = stack[--sp]; i = key & 0xffffff; - if (WARN_ON(i >= READ_BIB_ROM_SIZE)) + if (WARN_ON(i >= MAX_CONFIG_ROM_SIZE)) goto out; /* Read header quadlet for the block to get the length. */ if (read_rom(device, generation, i, &rom[i]) != RCODE_COMPLETE) goto out; end = i + (rom[i] >> 16) + 1; - if (end > READ_BIB_ROM_SIZE) { + if (end > MAX_CONFIG_ROM_SIZE) { /* * This block extends outside the config ROM which is * a firmware bug. Ignore this whole block, i.e. @@ -621,7 +621,7 @@ static int read_bus_info_block(struct fw * fake immediate entry so that later iterators over * the ROM don't have to check offsets all the time. */ - if (i + (rom[i] & 0xffffff) >= READ_BIB_ROM_SIZE) { + if (i + (rom[i] & 0xffffff) >= MAX_CONFIG_ROM_SIZE) { fw_error("skipped unsupported ROM entry %x at %llx\n", rom[i], i * 4 | CSR_REGISTER_BASE | CSR_CONFIG_ROM); @@ -971,7 +971,7 @@ static void fw_device_init(struct work_s * device. */ - if (read_bus_info_block(device, device->generation) < 0) { + if (read_config_rom(device, device->generation) < 0) { if (device->config_rom_retries < MAX_RETRIES && atomic_read(&device->state) == FW_DEVICE_INITIALIZING) { device->config_rom_retries++; @@ -1088,7 +1088,7 @@ enum { }; /* Reread and compare bus info block and header of root directory */ -static int reread_bus_info_block(struct fw_device *device, int generation) +static int reread_config_rom(struct fw_device *device, int generation) { u32 q; int i; @@ -1114,7 +1114,7 @@ static void fw_device_refresh(struct wor struct fw_card *card = device->card; int node_id = device->node_id; - switch (reread_bus_info_block(device, device->generation)) { + switch (reread_config_rom(device, device->generation)) { case REREAD_BIB_ERROR: if (device->config_rom_retries < MAX_RETRIES / 2 && atomic_read(&device->state) == FW_DEVICE_INITIALIZING) { @@ -1148,7 +1148,7 @@ static void fw_device_refresh(struct wor */ device_for_each_child(&device->device, NULL, shutdown_unit); - if (read_bus_info_block(device, device->generation) < 0) { + if (read_config_rom(device, device->generation) < 0) { if (device->config_rom_retries < MAX_RETRIES && atomic_read(&device->state) == FW_DEVICE_INITIALIZING) { device->config_rom_retries++; -- Stefan Richter -=====-==-=- --=- =--== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-02-19 20:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-18 0:50 [PATCH 1/3] firewire: core: fix "giving up on config rom" with Panasonic AG-DV2500 Stefan Richter 2010-02-18 0:52 ` [PATCH 2/3] firewire: core: don't fail device creation in case of too large config ROM blocks Stefan Richter 2010-02-18 0:54 ` [PATCH 3/3] firewire: core: increase stack size of config ROM reader Stefan Richter 2010-02-19 20:00 ` [PATCH] firewire: core: fix an information leak Stefan Richter 2010-02-19 20:00 ` [PATCH] firewire: core: rename an internal function Stefan Richter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.