Change in coreboot[master]: device/cardbus: Check for NULL

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Paul Menzel (Code Review)

20 Jul 2020 20 Jul '20

10:06 p.m.

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);

Attachments:

attachment.htm (text/html — 4.0 KB)

Show replies by date

Paul Menzel (Code Review)

20 Jul 20 Jul

10:07 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 20 Jul 2020 22:07:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Paul Menzel (Code Review)

10:08 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Angel Pons (Code Review)

10:14 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Mon, 20 Jul 2020 22:14:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

10:58 p.m.

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?

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Mon, 20 Jul 2020 22:58:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

11:35 p.m.

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");

...

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Mon, 20 Jul 2020 23:35:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Paul Menzel (Code Review)

21 Jul 21 Jul

6:26 a.m.

New subject: Change in coreboot[master]: device/cardbus: Check dev->link_list to avoid accessing garbage

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Nico Huber (Code Review)

10:46 p.m.

New subject: Change in coreboot[master]: device/cardbus: Check dev->link_list to avoid accessing garbage

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 21 Jul 2020 22:46:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

11:07 p.m.

New subject: Change in coreboot[master]: device/cardbus: Check dev->link_list to avoid accessing garbage

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 21 Jul 2020 23:07:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Nico Huber (Code Review)

23 Jul 23 Jul

8 p.m.

New subject: Change in coreboot[master]: device/cardbus: Check dev->link_list to avoid accessing garbage

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 23 Jul 2020 20:00:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Stefan Reinauer (Code Review)

10 Jun 10 Jun

3:08 a.m.

New subject: [S] Change in coreboot[master]: device/cardbus: Check dev->link_list to avoid accessing garbage

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/43656?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6f62229e6870897544b45d44278380bb5820416b Gerrit-Change-Number: 43656 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Menzel paulepanter@mailbox.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Paul Menzel paulepanter@mailbox.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: abandon

477

days inactive

1532

days old

coreboot-gerrit@coreboot.org

10 comments

4 participants

tags (0)

participants (4)

Angel Pons (Code Review)
Nico Huber (Code Review)
Paul Menzel (Code Review)
Stefan Reinauer (Code Review)