Change in coreboot[master]: drivers/ipmi: Avoid NULL pointer dereference

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Harshit Sharma (Code Review)

22 May 2020 22 May '20

8:35 a.m.

Harshit Sharma has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

drivers/ipmi: Avoid NULL pointer dereference

There are multiple instances where NULL pointer conf could be dereferenced. This patch fixes those issues.

Found-by: Coverity Scan #1407751, #1428709, #1428710, #1428714 Signed-off-by: Harshit Sharma harshitsharmajs@gmail.com Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 --- M src/drivers/ipmi/ipmi_kcs_ops.c 1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/41654/1

diff --git a/src/drivers/ipmi/ipmi_kcs_ops.c b/src/drivers/ipmi/ipmi_kcs_ops.c index 0b90fb2..e43f3a5 100644 --- a/src/drivers/ipmi/ipmi_kcs_ops.c +++ b/src/drivers/ipmi/ipmi_kcs_ops.c @@ -84,6 +84,9 @@ if (dev->chip_info) conf = dev->chip_info;

+ if (!conf) + return; + /* Get IPMI version for ACPI and SMBIOS */ if (conf && conf->wait_for_bmc && conf->bmc_boot_timeout) { struct stopwatch sw; @@ -200,6 +203,9 @@ if (dev->chip_info) conf = dev->chip_info;

+ if (!conf) + return 0; + if (conf) { if (conf->have_gpe) gpe_interrupt = conf->gpe_interrupt; @@ -234,6 +240,9 @@ if (dev->chip_info) conf = dev->chip_info;

+ if (!conf) + return; + /* Use command to pass UID to ipmi_write_acpi_tables */ conf->uid = uid_cnt++;

Attachments:

attachment.htm (text/html — 3.5 KB)

Show replies by date

Angel Pons (Code Review)

22 May 22 May

12:49 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1: Code-Review+1

(1 comment)

I'm not sure if we assume chip_info to never be NULL.

https://review.coreboot.org/c/coreboot/+/41654/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/41654/1//COMMIT_MSG@9 PS1, Line 9: dereferenced. Move this to the next line

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 22 May 2020 12:49:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Rudolph (Code Review)

1:34 p.m.

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1: Code-Review-1

That's not a proper fix.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 22 May 2020 13:34:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

4:13 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1: -Code-Review

...

Patch Set 1: Code-Review-1

That's not a proper fix.

Why so? What would be a proper fix?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 22 May 2020 16:13:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Rudolph (Code Review)

4:23 p.m.

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1:

Check the pointer before using it and handle that case gracefully. If we know that it's never going to be NULL you can use an assertion. That should also silence the static code analyser.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 22 May 2020 16:23:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Patrick Rudolph (Code Review)

4:24 p.m.

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... File src/drivers/ipmi/ipmi_kcs_ops.c:

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 91: conf That condition is now always true and thus obsolete

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 207: ; return current

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 209: conf Always true

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 22 May 2020 16:24:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

4:31 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... File src/drivers/ipmi/ipmi_kcs_ops.c:

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 254: acpigen_write_name_byte("_UID", conf->uid); Should we skip _UID and generate the rest of entries as usual?

https://review.coreboot.org/c/coreboot/+/41654/1/src/drivers/ipmi/ipmi_kcs_o... PS1, Line 262: if (conf) { always true

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 22 May 2020 16:31:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Harshit Sharma (Code Review)

24 May 24 May

6:56 a.m.

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 1:

...

Patch Set 1:

(3 comments)

Since conf will always be true, should we just put assert statements at those three places?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 1 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sun, 24 May 2020 06:56:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Harshit Sharma (Code Review)

25 May 25 May

6:41 a.m.

Hello build bot (Jenkins), Patrick Rudolph, Angel Pons, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41654

to look at the new patch set (#2).

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

drivers/ipmi: Avoid NULL pointer dereference

There are multiple instances where NULL pointer conf could be dereferenced. This patch fixes those issues.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/41654/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 2 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Harshit Sharma (Code Review)

8:28 a.m.

Hello build bot (Jenkins), Patrick Rudolph, Angel Pons, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41654

to look at the new patch set (#3).

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

drivers/ipmi: Avoid NULL pointer dereference

There are multiple instances where NULL pointer conf could be dereferenced. This patch fixes those issues.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/41654/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 3 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Bill XIE (Code Review)

12 Nov 12 Nov

3:04 a.m.

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 3:

How about to make code runnable even if dev->chip_info is NULL? Like my approach at I1d694b12f6c42961c104fe839d4ee46c0f111197 .

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 3 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Bill XIE persmule@hardenedlinux.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 12 Nov 2020 03:04:20 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Michael Niewöhner (Code Review)

20 Dec 20 Dec

12:55 a.m.

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 3:

Since CB:47387 is merged now, would you mind rerunning your coverty scans?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 3 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Bill XIE persmule@hardenedlinux.org Gerrit-CC: Michael Niewöhner foss@mniewoehner.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 20 Dec 2020 00:55:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Harshit Sharma (Code Review)

1:20 a.m.

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 3:

...

Patch Set 3:

Since CB:47387 is merged now, would you mind rerunning your coverty scans?

Yes, these issues have been fixed. I think I can abandon this change now.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 3 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Bill XIE persmule@hardenedlinux.org Gerrit-CC: Michael Niewöhner foss@mniewoehner.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 20 Dec 2020 01:20:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Michael Niewöhner (Code Review)

11:28 a.m.

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 3:

Since CB:47387 is merged now, would you mind rerunning your coverty scans?

Yes, these issues have been fixed. I think I can abandon this change now.

Great, thanks!

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 3 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Bill XIE persmule@hardenedlinux.org Gerrit-CC: Michael Niewöhner foss@mniewoehner.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 20 Dec 2020 11:28:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Harshit Sharma (Code Review)

6:42 p.m.

Harshit Sharma has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41654 )

Change subject: drivers/ipmi: Avoid NULL pointer dereference ......................................................................

Abandoned

-- To view, visit https://review.coreboot.org/c/coreboot/+/41654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d1cfe3f9b55288eeb55ab8785d857993e3989c0 Gerrit-Change-Number: 41654 Gerrit-PatchSet: 3 Gerrit-Owner: Harshit Sharma harshitsharmajs@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Bill XIE persmule@hardenedlinux.org Gerrit-CC: Michael Niewöhner foss@mniewoehner.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: abandon

1380

days inactive

1592

days old

coreboot-gerrit@coreboot.org

14 comments

5 participants

tags (0)

participants (5)

Angel Pons (Code Review)
Bill XIE (Code Review)
Harshit Sharma (Code Review)
Michael Niewöhner (Code Review)
Patrick Rudolph (Code Review)