John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
superio/nuvoton: Avoid NULL pointer dereference
Coverity detects dereferencing a pointer that might be "NULL" when calling acpigen_write_scope. Add sanity check for scope to prevent NULL pointer dereference.
Found-by: Coverity CID 1420207
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Icc253c63aadef1c0ecb116a38b608f64f80abc79 --- M src/superio/nuvoton/npcd378/superio.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42839/1
diff --git a/src/superio/nuvoton/npcd378/superio.c b/src/superio/nuvoton/npcd378/superio.c index 628bd4e..46923e0 100644 --- a/src/superio/nuvoton/npcd378/superio.c +++ b/src/superio/nuvoton/npcd378/superio.c @@ -325,7 +325,8 @@ acpigen_pop_len(); /* Pop Scope */
/* Inject into parent: */ - acpigen_write_scope(acpi_device_scope(dev)); + if (scope) + acpigen_write_scope(scope);
acpigen_write_name_integer("MSFG", 1); acpigen_write_name_integer("KBFG", 1);
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/1/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/1/src/superio/nuvoton/npcd378... PS1, Line 329: acpigen_write_scope(scope); just not writing the scope, but the rest from this scope will likely result in broken AML code being generated
Hello build bot (Jenkins), Paul Menzel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42839
to look at the new patch set (#2).
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
superio/nuvoton: Avoid NULL pointer dereference
Coverity detects dereferencing a pointer that might be "NULL" when calling acpigen_write_scope. Add sanity check for scope to prevent NULL pointer dereference.
Found-by: Coverity CID 1420207
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Icc253c63aadef1c0ecb116a38b608f64f80abc79 --- M src/superio/nuvoton/npcd378/superio.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42839/2
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/1/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/1/src/superio/nuvoton/npcd378... PS1, Line 329: acpigen_write_scope(scope);
just not writing the scope, but the rest from this scope will likely result in broken AML code being […]
Just updated to abort if scope is NULL.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/2/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/2/src/superio/nuvoton/npcd378... PS2, Line 329: return; maybe also print an error to the console?
Hello build bot (Jenkins), Paul Menzel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42839
to look at the new patch set (#3).
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
superio/nuvoton: Avoid NULL pointer dereference
Coverity detects dereferencing a pointer that might be "NULL" when calling acpigen_write_scope. Add sanity check for scope to prevent NULL pointer dereference.
Found-by: Coverity CID 1420207
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Icc253c63aadef1c0ecb116a38b608f64f80abc79 --- M src/superio/nuvoton/npcd378/superio.c 1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42839/3
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/2/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/2/src/superio/nuvoton/npcd378... PS2, Line 329: return;
maybe also print an error to the console?
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... PS3, Line 329: printk(BIOS_ERR, "Device scope does not exist.\n"); Error messages should be understandable by a “normal” user. Please add some kind of prefix, and add the effect, like that the ACPI table might be incomplete or so.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... PS3, Line 410: { How about adding the null check here?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... PS3, Line 329: printk(BIOS_ERR, "Device scope does not exist.\n");
Error messages should be understandable by a “normal” user. […]
Done
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... PS3, Line 410: {
How about adding the null check here?
It seems not relevant because there is already NULL check for scope in the function superio_common_fill_ssdt_generator. The scope NULL check is only applied to acpigen_write_scope call.
Hello build bot (Jenkins), Paul Menzel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42839
to look at the new patch set (#4).
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
superio/nuvoton: Avoid NULL pointer dereference
Coverity detects dereferencing a pointer that might be "NULL" when calling acpigen_write_scope. Add sanity check for scope to prevent NULL pointer dereference.
Found-by: Coverity CID 1420207
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Icc253c63aadef1c0ecb116a38b608f64f80abc79 --- M src/superio/nuvoton/npcd378/superio.c 1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42839/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... PS3, Line 410: {
It seems not relevant because there is already NULL check for scope in the function superio_common_fill_ssdt_generator. The scope NULL check is only applied to acpigen_write_scope call.
What I mean is that adding a null check in this function also prevents warnings about `dev` being null in all `npcd378_ssdt_*` functions called from here.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... File src/superio/nuvoton/npcd378/superio.c:
https://review.coreboot.org/c/coreboot/+/42839/3/src/superio/nuvoton/npcd378... PS3, Line 410: {
It seems not relevant because there is already NULL check for scope in the function superio_common […]
got it.
Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42839
to look at the new patch set (#5).
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
superio/nuvoton: Avoid NULL pointer dereference
Coverity detects dereferencing a pointer that might be "NULL" when calling acpigen_write_scope. Add sanity check for scope to prevent NULL pointer dereference.
Found-by: Coverity CID 1420207
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Icc253c63aadef1c0ecb116a38b608f64f80abc79 --- M src/superio/nuvoton/npcd378/superio.c 1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42839/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 5: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42839 )
Change subject: superio/nuvoton: Avoid NULL pointer dereference ......................................................................
superio/nuvoton: Avoid NULL pointer dereference
Coverity detects dereferencing a pointer that might be "NULL" when calling acpigen_write_scope. Add sanity check for scope to prevent NULL pointer dereference.
Found-by: Coverity CID 1420207
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Icc253c63aadef1c0ecb116a38b608f64f80abc79 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42839 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/superio/nuvoton/npcd378/superio.c 1 file changed, 8 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/superio/nuvoton/npcd378/superio.c b/src/superio/nuvoton/npcd378/superio.c index 628bd4e..b7f98af 100644 --- a/src/superio/nuvoton/npcd378/superio.c +++ b/src/superio/nuvoton/npcd378/superio.c @@ -325,7 +325,11 @@ acpigen_pop_len(); /* Pop Scope */
/* Inject into parent: */ - acpigen_write_scope(acpi_device_scope(dev)); + if (!scope) { + printk(BIOS_ERR, "%s: Missing ACPI path/scope\n", dev_path(dev)); + return; + } + acpigen_write_scope(scope);
acpigen_write_name_integer("MSFG", 1); acpigen_write_name_integer("KBFG", 1); @@ -404,6 +408,9 @@
static void npcd378_fill_ssdt_generator(const struct device *dev) { + if (!dev) + return; + superio_common_fill_ssdt_generator(dev);
switch (dev->path.pnp.device) {