Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check for NULL ......................................................................
device/cardbus: Check for NULL
While debugging problems on a Lenovo T60 device, Kyösti noticed a NULL pointer access. Check before accessing garbage value.
Change-Id: I6f62229e6870897544b45d44278380bb5820416b Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M 3rdparty/blobs M src/device/cardbus_device.c 2 files changed, 10 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/43656/1
diff --git a/3rdparty/blobs b/3rdparty/blobs index bbe5d99..7ad2d22 160000 --- a/3rdparty/blobs +++ b/3rdparty/blobs @@ -1 +1 @@ -Subproject commit bbe5d99780d2d085e92d9bae2c0f7b6787419d72 +Subproject commit 7ad2d22452225a14c19b17570cb77920d8fc81a5 diff --git a/src/device/cardbus_device.c b/src/device/cardbus_device.c index cbe0c72..5fe2296 100644 --- a/src/device/cardbus_device.c +++ b/src/device/cardbus_device.c @@ -139,11 +139,15 @@ u16 ctrl;
ctrl = pci_read_config16(dev, PCI_CB_BRIDGE_CONTROL); - ctrl |= (dev->link_list->bridge_ctrl & ( - PCI_BRIDGE_CTL_NO_ISA | - PCI_BRIDGE_CTL_VGA | - PCI_BRIDGE_CTL_MASTER_ABORT | - PCI_BRIDGE_CTL_BUS_RESET)); + if (dev->link_list) + ctrl |= (dev->link_list->bridge_ctrl & ( + PCI_BRIDGE_CTL_NO_ISA | + PCI_BRIDGE_CTL_VGA | + PCI_BRIDGE_CTL_MASTER_ABORT | + PCI_BRIDGE_CTL_BUS_RESET)); + else + printk(BIOS_ERR, "cardbus: dev->link_list is NULL\n"); + /* Error check */ ctrl |= (PCI_CB_BRIDGE_CTL_PARITY | PCI_CB_BRIDGE_CTL_SERR); printk(BIOS_DEBUG, "%s bridge ctrl <- %04x\n", dev_path(dev), ctrl);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check for NULL ......................................................................
Patch Set 1: Code-Review-1
Commit message needs to be written.
Hello Nico Huber, Angel Pons, Kyösti Mälkki,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43656
to look at the new patch set (#2).
Change subject: device/cardbus: Check for NULL ......................................................................
device/cardbus: Check for NULL
While debugging problems on a Lenovo T60 device, Kyösti noticed a NULL pointer access. Check before accessing garbage value.
Change-Id: I6f62229e6870897544b45d44278380bb5820416b Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/device/cardbus_device.c 1 file changed, 9 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/43656/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check for NULL ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c... PS2, Line 149: printk(BIOS_ERR, "cardbus: dev->link_list is NULL\n"); I'd return here.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check for NULL ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c... PS2, Line 147: PCI_BRIDGE_CTL_BUS_RESET)); Nit, I would prefer braces around this.
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c... PS2, Line 149: printk(BIOS_ERR, "cardbus: dev->link_list is NULL\n");
I'd return here.
And skip the working code below? Most of all, not enable needed resources?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check for NULL ......................................................................
Patch Set 2:
(1 comment)
a
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c... PS2, Line 149: printk(BIOS_ERR, "cardbus: dev->link_list is NULL\n");
And skip the working code below? Most of all, not enable needed resources?
Well, if link_list isn't valid, I'm not sure how the code would handle it.
Hello build bot (Jenkins), Nico Huber, Angel Pons, Kyösti Mälkki,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43656
to look at the new patch set (#3).
Change subject: device/cardbus: Check dev->link_list to avoid accessing garbage ......................................................................
device/cardbus: Check dev->link_list to avoid accessing garbage
While debugging problems on a Lenovo T60 device, Kyösti noticed a NULL pointer access. `dev->link_list` can be NULL, as resource scanning `scan_resources` is not implemented. So, add a check to avoid a NULL pointer access.
Change-Id: I6f62229e6870897544b45d44278380bb5820416b Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/device/cardbus_device.c 1 file changed, 11 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/43656/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check dev->link_list to avoid accessing garbage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c... PS2, Line 149: printk(BIOS_ERR, "cardbus: dev->link_list is NULL\n");
Well, if link_list isn't valid, I'm not sure how the code would handle it.
It's only the 4 lines below. Not hard to read, is it? In total what this function does: * Set additional bits in the bridge control reg. * Allow `link_list->bridge_ctrl` to add some more bits. If there is no `link_list`, we can still go on as if there was one that wouldn't set more bits.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check dev->link_list to avoid accessing garbage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/43656/2/src/device/cardbus_device.c... PS2, Line 149: printk(BIOS_ERR, "cardbus: dev->link_list is NULL\n");
It's only the 4 lines below. Not hard to read, is it? In total what this […]
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43656 )
Change subject: device/cardbus: Check dev->link_list to avoid accessing garbage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43656/3/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/43656/3/src/device/cardbus_device.c... PS3, Line 150: printk(BIOS_INFO, "cardbus: dev->link_list is NULL, as scan_resources is missing\n"); It's `.scan_bus` that's missing in case of the TI driver and in theory it's possible that it's NULL for other reasons, too, e.g. a bug.
The old message was good. But I agree that BIOS_ERR is a bit too much.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43656?usp=email )
Change subject: device/cardbus: Check dev->link_list to avoid accessing garbage ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)