Change in ...coreboot[master]: sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads
Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33790
Change subject: sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads ......................................................................
sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads
"There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors."
var_num records the number of initialized entries in the reg_var array. However, this means the index of the last initialized element is one less than the value of var_num, so we need to take that into account when indexing into the array. This has already been fixed in several other places (eg. sb/amd/pi/hudson/lpc.c), so let's also do so here.
Change-Id: Ibefabaca42866a3f2b22eff979c73badf86ac317 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: scan-build 8.0.0 --- M src/southbridge/amd/cimx/sb800/lpc.c M src/southbridge/amd/cimx/sb900/lpc.c M src/southbridge/amd/sb700/lpc.c M src/southbridge/amd/sb800/lpc.c 4 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/33790/1
diff --git a/src/southbridge/amd/cimx/sb800/lpc.c b/src/southbridge/amd/cimx/sb800/lpc.c index a88d6d3..483d185 100644 --- a/src/southbridge/amd/cimx/sb800/lpc.c +++ b/src/southbridge/amd/cimx/sb800/lpc.c @@ -170,11 +170,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: //pci_write_config16(dev, 0x64, reg_var[0]); //cause filo can not find sata break; } diff --git a/src/southbridge/amd/cimx/sb900/lpc.c b/src/southbridge/amd/cimx/sb900/lpc.c index b04ecfa..8fcb947 100644 --- a/src/southbridge/amd/cimx/sb900/lpc.c +++ b/src/southbridge/amd/cimx/sb900/lpc.c @@ -168,11 +168,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: //pci_write_config16(dev, 0x64, reg_var[0]); //cause filo can not find sata break; } diff --git a/src/southbridge/amd/sb700/lpc.c b/src/southbridge/amd/sb700/lpc.c index 6f3be03..b7f0dc3 100644 --- a/src/southbridge/amd/sb700/lpc.c +++ b/src/southbridge/amd/sb700/lpc.c @@ -228,11 +228,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: pci_write_config16(dev, 0x64, reg_var[0]); break; } diff --git a/src/southbridge/amd/sb800/lpc.c b/src/southbridge/amd/sb800/lpc.c index 649add5..74b6374 100644 --- a/src/southbridge/amd/sb800/lpc.c +++ b/src/southbridge/amd/sb800/lpc.c @@ -220,11 +220,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: pci_write_config16(dev, 0x64, reg_var[0]); break; }
Hello Kyösti Mälkki, Angel Pons, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33790
to look at the new patch set (#2).
Change subject: sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads ......................................................................
sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads
There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. -- Anonymous
var_num records the number of initialized entries in the reg_var array. However, this means the index of the last initialized element is one less than the value of var_num, so we need to take that into account when indexing into the array. This has already been fixed in several other places (eg. sb/amd/pi/hudson/lpc.c), so let's also do so here.
Change-Id: Ibefabaca42866a3f2b22eff979c73badf86ac317 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: scan-build 8.0.0 --- M src/southbridge/amd/cimx/sb800/lpc.c M src/southbridge/amd/cimx/sb900/lpc.c M src/southbridge/amd/sb700/lpc.c M src/southbridge/amd/sb800/lpc.c 4 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/33790/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33790 )
Change subject: sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33790 )
Change subject: sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads ......................................................................
Patch Set 2: Code-Review+1
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33790 )
Change subject: sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads ......................................................................
sb/amd/{cimx,}/sb{700,800,900}: Prevent uninitialized reads
There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. -- Anonymous
var_num records the number of initialized entries in the reg_var array. However, this means the index of the last initialized element is one less than the value of var_num, so we need to take that into account when indexing into the array. This has already been fixed in several other places (eg. sb/amd/pi/hudson/lpc.c), so let's also do so here.
Change-Id: Ibefabaca42866a3f2b22eff979c73badf86ac317 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: scan-build 8.0.0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33790 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: David Hendricks david.hendricks@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/southbridge/amd/cimx/sb800/lpc.c M src/southbridge/amd/cimx/sb900/lpc.c M src/southbridge/amd/sb700/lpc.c M src/southbridge/amd/sb800/lpc.c 4 files changed, 12 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/src/southbridge/amd/cimx/sb800/lpc.c b/src/southbridge/amd/cimx/sb800/lpc.c index a88d6d3..483d185 100644 --- a/src/southbridge/amd/cimx/sb800/lpc.c +++ b/src/southbridge/amd/cimx/sb800/lpc.c @@ -170,11 +170,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: //pci_write_config16(dev, 0x64, reg_var[0]); //cause filo can not find sata break; } diff --git a/src/southbridge/amd/cimx/sb900/lpc.c b/src/southbridge/amd/cimx/sb900/lpc.c index b04ecfa..8fcb947 100644 --- a/src/southbridge/amd/cimx/sb900/lpc.c +++ b/src/southbridge/amd/cimx/sb900/lpc.c @@ -168,11 +168,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: //pci_write_config16(dev, 0x64, reg_var[0]); //cause filo can not find sata break; } diff --git a/src/southbridge/amd/sb700/lpc.c b/src/southbridge/amd/sb700/lpc.c index 6f3be03..b7f0dc3 100644 --- a/src/southbridge/amd/sb700/lpc.c +++ b/src/southbridge/amd/sb700/lpc.c @@ -228,11 +228,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: pci_write_config16(dev, 0x64, reg_var[0]); break; } diff --git a/src/southbridge/amd/sb800/lpc.c b/src/southbridge/amd/sb800/lpc.c index 649add5..74b6374 100644 --- a/src/southbridge/amd/sb800/lpc.c +++ b/src/southbridge/amd/sb800/lpc.c @@ -220,11 +220,11 @@ pci_write_config32(dev, 0x48, reg_x); /* Set WideIO for as many IOs found (fall through is on purpose) */ switch (var_num) { - case 2: + case 3: pci_write_config16(dev, 0x90, reg_var[2]); - case 1: + case 2: pci_write_config16(dev, 0x66, reg_var[1]); - case 0: + case 1: pci_write_config16(dev, 0x64, reg_var[0]); break; }
-
David Hendricks (Code Review) -
Jacob Garber (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)