Change in coreboot[master]: src/device/dram/ddr4: Fix "Dead assignment"

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35558 )

Change subject: src/device/dram/ddr4: Fix "Dead assignment"
......................................................................

src/device/dram/ddr4: Fix "Dead assignment"

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/1

diff --git a/src/device/dram/ddr4.c b/src/device/dram/ddr4.c
index 4f99ecc..472a771 100644
--- a/src/device/dram/ddr4.c
+++ b/src/device/dram/ddr4.c
@@ -120,6 +120,13 @@
    }

spd_bytes_total = 256 << (spd_bytes_total - 1);
+
+	if (spd_bytes_total > 512)
+		printk(BIOS_WARNING, "SPD Bytes Total value is reserved\n");
+
+	if (spd_bytes_used > 4)
+		printk(BIOS_WARNING, "SPD Bytes Used value is reserved\n");
+
    spd_bytes_used = spd_bytes_used_table[spd_bytes_used];

/* Verify CRC of blocks that have them, do not step over 'used' length */

[Patrick Rudolph (Code Review)]

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

Change subject: src/device/dram/ddr4: Fix "Dead assignment"
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35558/1/src/device/dram/ddr4.c 
File src/device/dram/ddr4.c:

https://review.coreboot.org/c/coreboot/+/35558/1/src/device/dram/ddr4.c@127 
PS1, Line 127: 	if (spd_bytes_used > 4)
that causes an out of bound access. Better return here.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/35558 )

Change subject: src/device/dram/ddr4: Fix "Dead assignment"
......................................................................

src/device/dram/ddr4: Fix "Dead assignment"

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/2

[Nico Huber (Code Review)]

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

Change subject: src/device/dram/ddr4: Fix "Dead assignment"
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/35558/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35558/2//COMMIT_MSG@7 
PS2, Line 7: src/device/dram/ddr4: Fix "Dead assignment"
No src/ prefix, please.

I know it's hard to find a good summary that covers independent
changes... but please try to, or split the changes. "Fix Dead
assignment" is clearly not the only thing this change does.

Now that the `return` was added, that is clearly the more important
change here! For one thing, it fixes a really bad bug, and for another,
it changes behaviour (what fixing a dead assignment doesn't).

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes are reserved

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Nico Huber,

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

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

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes are reserved
......................................................................

device/dram/ddr4: Check if total or used SPD bytes are reserved

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.
If 'SPD Bytes Used' value is reserved return SPD_STATUS_INVALID as it
causes an out of bound access.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/3

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes are reserved

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Nico Huber,

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

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

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes are reserved
......................................................................

device/dram/ddr4: Check if total or used SPD bytes are reserved

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.
If 'SPD Bytes Used' value is reserved, then return SPD_STATUS_INVALID as it
causes an out of bound access.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/4

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Nico Huber,

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

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

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

device/dram/ddr4: Check if total or used SPD bytes value is reserved

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.
If 'SPD Bytes Used' value is reserved, then return SPD_STATUS_INVALID as it
causes an out of bound access.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/5

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[HAOUAS Elyes (Code Review)]

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35558/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35558/2//COMMIT_MSG@7 
PS2, Line 7: src/device/dram/ddr4: Fix "Dead assignment"
> No src/ prefix, please. […]

Done

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35558 )

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

Patch Set 6:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c 
File src/device/dram/ddr4.c:

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c@113 
PS6, Line 113: 
             : 	spd_bytes_total = (spd[0] >> 4) & ((1 << 3) - 1);
             : 	spd_bytes_used = spd[0] & ((1 << 4) - 1);
Feel free to use proper hex for masks here.

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c@121 
PS6, Line 121: 
             : 	spd_bytes_total = 256 << (spd_bytes_total - 1);
             : 
             : 	if (spd_bytes_total > 512)
             : 		printk(BIOS_WARNING, "SPD Bytes Total value is reserved\n");
The value '3' in bytes_total is reserved so check for that instead of a meaningless decoded value.

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c@132 
PS6, Line 132: spd_bytes_used
You could compare this to be smaller than spd_bytes_total as a sanity check

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Nico Huber,

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

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

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

device/dram/ddr4: Check if total or used SPD bytes value is reserved

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.
If 'SPD Bytes Used' value is reserved, then return SPD_STATUS_INVALID as it
causes an out of bound access.
Add a sanity check if used bytes is greater than the total number of bytes.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/7

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[HAOUAS Elyes (Code Review)]

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

Patch Set 7:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c 
File src/device/dram/ddr4.c:

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c@113 
PS6, Line 113: 
             : 	spd_bytes_total = (spd[0] >> 4) & ((1 << 3) - 1);
             : 	spd_bytes_used = spd[0] & ((1 << 4) - 1);
> Feel free to use proper hex for masks here.

Done

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c@121 
PS6, Line 121: 
             : 	spd_bytes_total = 256 << (spd_bytes_total - 1);
             : 
             : 	if (spd_bytes_total > 512)
             : 		printk(BIOS_WARNING, "SPD Bytes Total value is reserved\n");
> The value '3' in bytes_total is reserved so check for that instead of a meaningless decoded value.

Done

https://review.coreboot.org/c/coreboot/+/35558/6/src/device/dram/ddr4.c@132 
PS6, Line 132: spd_bytes_used
> You could compare this to be smaller than spd_bytes_total as a sanity check

Done

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Nico Huber,

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

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

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

device/dram/ddr4: Check if total or used SPD bytes value is reserved

The value stored to 'spd_bytes_total' is never read, so let
use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
reserved.
If 'SPD Bytes Used' value is reserved, then return SPD_STATUS_INVALID as it
causes an out of bound access.
Add a sanity check if used bytes is greater than the total number of bytes.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/8

Change in coreboot[master]: device/dram/ddr4: Check if total or used SPD bytes value is reserved

[Nico Huber (Code Review)]

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

Change subject: device/dram/ddr4: Check if total or used SPD bytes value is reserved
......................................................................

Patch Set 8: Code-Review+2

(1 comment)

The commit message looks like badly formatted code in my browser:

device/dram/ddr4: Check if total or used SPD bytes value is reserved

The value stored to 'spd_bytes_total' is never read, so let
    use it to check if 'SPD Bytes Total' and/or 'SPD Bytes Used' are
    reserved.
    If 'SPD Bytes Used' value is reserved, then return SPD_STATUS_INVALID as
    it
    causes an out of bound access.
    Add a sanity check if used bytes is greater than the total number of
    bytes.

Please! If you want to make a list, make a list. If you want to write
a paragraph, don't add linebreaks after every full stop. And if you
want it to be viewable in standard programs (like Gerrit), obey the
72-chars-per-line rule of Git.

https://review.coreboot.org/c/coreboot/+/35558/8//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35558/8//COMMIT_MSG@10 
PS8, Line 10: and/or 'SPD Bytes Used'
this is not really what is happening, `spd_bytes_total` is obviously
_not_ used to check if `SPD Bytes Used` is reserved.

Change in coreboot[master]: device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Nico Huber,

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

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

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

Change subject: device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values
......................................................................

device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values

The value stored to 'spd_bytes_total' is never read. Now it is fixed.
This is spotted using clang-tool v9.
Also add a check if spd_bytes_used and/or spd_bytes_total are reserved
and make sure that spd_bytes_used is not greater than spd_bytes_total.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
---
M src/device/dram/ddr4.c
1 file changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35558/9

Change in coreboot[master]: device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values

[Nico Huber (Code Review)]

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

Change subject: device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values
......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35558/1/src/device/dram/ddr4.c 
File src/device/dram/ddr4.c:

https://review.coreboot.org/c/coreboot/+/35558/1/src/device/dram/ddr4.c@127 
PS1, Line 127: 	if (spd_bytes_used > 4)
> that causes an out of bound access. Better return here.

Done

https://review.coreboot.org/c/coreboot/+/35558/9/src/device/dram/ddr4.c 
File src/device/dram/ddr4.c:

https://review.coreboot.org/c/coreboot/+/35558/9/src/device/dram/ddr4.c@132 
PS9, Line 132: 	spd_bytes_used = spd_bytes_used_table[spd_bytes_used];
Could turn this into a calculation, too. It's `x * 128`, isn't it?

Change in coreboot[master]: device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35558 )

Change subject: device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values
......................................................................

device/dram/ddr4: Check spd_bytes_total and spd_bytes_used values

The value stored to 'spd_bytes_total' is never read. Now it is fixed.
This is spotted using clang-tool v9.
Also add a check if spd_bytes_used and/or spd_bytes_total are reserved
and make sure that spd_bytes_used is not greater than spd_bytes_total.

Change-Id: I426a7e64cc4c0bcced91d03387e02c8d965a21dc
Signed-off-by: Elyes HAOUAS 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35558
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/device/dram/ddr4.c
1 file changed, 16 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/device/dram/ddr4.c b/src/device/dram/ddr4.c
index 4f99ecc..07f9dec 100644
--- a/src/device/dram/ddr4.c
+++ b/src/device/dram/ddr4.c
@@ -111,17 +111,31 @@
    	return SPD_STATUS_INVALID;
    }

-	spd_bytes_total = (spd[0] >> 4) & ((1 << 3) - 1);
-	spd_bytes_used = spd[0] & ((1 << 4) - 1);
+	spd_bytes_total = (spd[0] >> 4) & 0x7;
+	spd_bytes_used = spd[0] & 0xf;

if (!spd_bytes_total || !spd_bytes_used) {
    	printk(BIOS_ERR, "SPD failed basic sanity checks\n");
    	return SPD_STATUS_INVALID;
    }

+	if (spd_bytes_total >= 3)
+		printk(BIOS_WARNING, "SPD Bytes Total value is reserved\n");
+
    spd_bytes_total = 256 << (spd_bytes_total - 1);
+
+	if (spd_bytes_used > 4) {
+		printk(BIOS_ERR, "SPD Bytes Used value is reserved\n");
+		return SPD_STATUS_INVALID;
+	}
+
    spd_bytes_used = spd_bytes_used_table[spd_bytes_used];

+	if (spd_bytes_used > spd_bytes_total) {
+		printk(BIOS_ERR, "SPD Bytes Used is greater than SPD Bytes Total\n");
+		return SPD_STATUS_INVALID;
+	}
+
    /* Verify CRC of blocks that have them, do not step over 'used' length */
    for (int i = 0; i < ARRAY_SIZE(spd_blocks); i++) {
    	/* this block is not checksumed */