Change in ...coreboot[master]: sb/intel/common: Use correct bitwise operator

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33963

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

sb/intel/common: Use correct bitwise operator

Like the line above it, this should be & instead of | (otherwise it will
always incorrectly return true).

Change-Id: I5208b523c815d15d7263594f06ccfacd8a9510b1
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1402092
---
M src/southbridge/intel/common/spi.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/33963/1

diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c
index e8c8f01..e9e66dc 100644
--- a/src/southbridge/intel/common/spi.c
+++ b/src/southbridge/intel/common/spi.c
@@ -338,7 +338,7 @@
    if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) {
    	return !!(readw_(&cntlr->ich7_spi->spis) & HSFS_FLOCKDN);
    } else {
-		return !!(readw_(&cntlr->ich9_spi->hsfs) | HSFS_FLOCKDN);
+		return !!(readw_(&cntlr->ich9_spi->hsfs) & HSFS_FLOCKDN);
    }
 }

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33963 )

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

Patch Set 1: Code-Review+2

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33963 )

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

Patch Set 1: Code-Review+1

Maybe improve commit message with possible security implications.

It was not wrong for long, two weeks, CB:33388.

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33963 )

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+1
>
> Maybe improve commit message with possible security implications.
>
> It was not wrong for long, two weeks, CB:33388.

Hmmm, I don't really know enough to know what those would be.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33963 )

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

Patch Set 1: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33963 )

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

Patch Set 1: Code-Review+2
> > Maybe improve commit message with possible security implications.
> >
> > It was not wrong for long, two weeks, CB:33388.
>
> Hmmm, I don't really know enough to know what those would be.

spi_locked() is only used locally to decide which path to take
(reprogram opcodes on the fly or live with the preset, locked
opcodes; IMHO, that we have two paths is a bug anyway). I don't
see any security problems, though even that is worth to mention,
e.g.

spi_locked() is only used internally to decide which opcodes
    will be used to talk the flash. If it is falsely reported as
    locked, worst case should be a denial of service (unless there
    are more bugs).

Change in coreboot[master]: sb/intel/common: Use correct bitwise operator

[Jacob Garber (Code Review)]

Hello Kyösti Mälkki, HAOUAS Elyes, Arthur Heymans, Paul Menzel, David Hendricks, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,

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

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

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

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

sb/intel/common: Use correct bitwise operator

Like the line above it, this should be & instead of | (otherwise it will
always incorrectly return true). spi_locked() is only used internally to
decide which opcodes will be used to talk to the flash, and if it is
falsely reported as locked, the worst case should be a denial of service
(unless there are more bugs).

Change-Id: I5208b523c815d15d7263594f06ccfacd8a9510b1
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1402092
---
M src/southbridge/intel/common/spi.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/33963/2

Change in coreboot[master]: sb/intel/common: Use correct bitwise operator

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33963 )

Change subject: sb/intel/common: Use correct bitwise operator
......................................................................

sb/intel/common: Use correct bitwise operator

Like the line above it, this should be & instead of | (otherwise it will
always incorrectly return true). spi_locked() is only used internally to
decide which opcodes will be used to talk to the flash, and if it is
falsely reported as locked, the worst case should be a denial of service
(unless there are more bugs).

Change-Id: I5208b523c815d15d7263594f06ccfacd8a9510b1
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1402092
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33963
Tested-by: build bot (Jenkins) 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Kyösti Mälkki 
Reviewed-by: Paul Menzel 
Reviewed-by: Nico Huber 
---
M src/southbridge/intel/common/spi.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, but someone else must approve
  Nico Huber: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve
  HAOUAS Elyes: Looks good to me, approved

diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c
index e8c8f01..e9e66dc 100644
--- a/src/southbridge/intel/common/spi.c
+++ b/src/southbridge/intel/common/spi.c
@@ -338,7 +338,7 @@
    if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) {
    	return !!(readw_(&cntlr->ich7_spi->spis) & HSFS_FLOCKDN);
    } else {
-		return !!(readw_(&cntlr->ich9_spi->hsfs) | HSFS_FLOCKDN);
+		return !!(readw_(&cntlr->ich9_spi->hsfs) & HSFS_FLOCKDN);
    }
 }