Change in ...coreboot[master]: nb/via/vx900: Use 64 bits to prevent overflow

[Jacob Garber (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

nb/via/vx900: Use 64 bits to prevent overflow

The bit operations are currently done using 32 bit math.
Cast the first argument to 64 bits to prevent possible
overflow.

Found-by: Coverity Scan, CID 1229665, 1229666
Signed-off-by: Jacob Garber 
Change-Id: Idd180f31e8cff797a6499b12bc685daa993aae05
---
M src/northbridge/via/vx900/northbridge.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32291/1

diff --git a/src/northbridge/via/vx900/northbridge.c b/src/northbridge/via/vx900/northbridge.c
index d865f38..dcb7fd5 100644
--- a/src/northbridge/via/vx900/northbridge.c
+++ b/src/northbridge/via/vx900/northbridge.c
@@ -266,8 +266,8 @@
     * to be always mapped to the top of 1M, but this can be overcome with
     * some smart positive/subtractive resource decoding */
    ram_resource(dev, idx++, 768, (tolmk - 768));
-	uma_memory_size = fbufk << 10;
-	uma_memory_base = tolmk << 10;
+	uma_memory_size = (uint64_t)fbufk << 10;
+	uma_memory_base = (uint64_t)tolmk << 10;

//uma_resource(dev, idx++, uma_memory_base>>10, uma_memory_size>>10);

[Arthur Heymans (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
Does this fix anything? If the 32 bit math overflows you'd certainly still have problems with that cast. IMO it would make more sense to check for the sanity of the memory space sizes.

[Jacob Garber (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
> Does this fix anything? If the 32 bit math overflows you'd certainly still have problems with that c […]

What we currently have is

64 bit variable = 32 bit variable << 10

The problem is that the bit shift is done using a 32 bit intermediate value (which could overflow) and is then extended to 64 bits. The cast forces a 64 bit intermediate value to be used, which will always have enough space to hold the computation.

[Arthur Heymans (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
> What we currently have is […]

I can see that, but a cast does not fix anything, you just change the nature of the  problem. What you ought to do, is do a sanity check on the 32 bit variable. For instance you have a value that needs to be below 4 << 20 (below 4G in KiB). Checking that makes more sense than avoiding overflows, because overflow or no overflow with the cast, you are in trouble...

[Angel Pons (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
> I can see that, but a cast does not fix anything, you just change the nature of the  problem. […]

Doesn't coreboot run in 32-bit mode anyway?

Change in coreboot[master]: nb/via/vx900: Use 64 bits to prevent overflow

[Jacob Garber (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
> Doesn't coreboot run in 32-bit mode anyway?

Ok, I'd like to resurrect this issue. Which variables should have the sanity checks? fbufk and tolmk?

Change in coreboot[master]: nb/via/vx900: Use 64 bits to prevent overflow

[Patrick Georgi (Code Review)]

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

Change subject: nb/via/vx900: Use 64 bits to prevent overflow
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
> Ok, I'd like to resurrect this issue. […]

That's how I understand Arthur, and I agree. Check those for being small enough that << 10 doesn't overflow and things work out.

Change in coreboot[master]: nb/via/vx900: Ensure memory size and base are in range

[Jacob Garber (Code Review)]

Hello Kyösti Mälkki, HAOUAS Elyes, build bot (Jenkins),

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

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

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

Change subject: nb/via/vx900: Ensure memory size and base are in range
......................................................................

nb/via/vx900: Ensure memory size and base are in range

We need to ensure uma_memory_size and uma_memory_base stay within a
32-bit address range. Both of these variables are 64 bits wide, so it is
simplest to use 64 bit math when doing the bit shifts and then check if
they are in range after.

Change-Id: Idd180f31e8cff797a6499b12bc685daa993aae05
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229665, 1229666
---
M src/northbridge/via/vx900/northbridge.c
1 file changed, 9 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32291/2

Change in coreboot[master]: nb/via/vx900: Ensure memory size and base are in range

[Jacob Garber (Code Review)]

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

Change subject: nb/via/vx900: Ensure memory size and base are in range
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/32291/1//COMMIT_MSG@9 
PS1, Line 9: The bit operations are currently done using 32 bit math.
           : Cast the first argument to 64 bits to prevent possible
           : overflow.
> That's how I understand Arthur, and I agree. […]

I added the check, but did it after the bit shifts to keep things simple.

Change in coreboot[master]: nb/via/vx900: Ensure memory size and base are in range

[Patrick Georgi (Code Review)]

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

Change subject: nb/via/vx900: Ensure memory size and base are in range
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: nb/via/vx900: Ensure memory size and base are in range

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32291 )

Change subject: nb/via/vx900: Ensure memory size and base are in range
......................................................................

nb/via/vx900: Ensure memory size and base are in range

We need to ensure uma_memory_size and uma_memory_base stay within a
32-bit address range. Both of these variables are 64 bits wide, so it is
simplest to use 64 bit math when doing the bit shifts and then check if
they are in range after.

Change-Id: Idd180f31e8cff797a6499b12bc685daa993aae05
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229665, 1229666
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32291
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M src/northbridge/via/vx900/northbridge.c
1 file changed, 9 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/northbridge/via/vx900/northbridge.c b/src/northbridge/via/vx900/northbridge.c
index d865f38..260bd3d 100644
--- a/src/northbridge/via/vx900/northbridge.c
+++ b/src/northbridge/via/vx900/northbridge.c
@@ -266,8 +266,15 @@
     * to be always mapped to the top of 1M, but this can be overcome with
     * some smart positive/subtractive resource decoding */
    ram_resource(dev, idx++, 768, (tolmk - 768));
-	uma_memory_size = fbufk << 10;
-	uma_memory_base = tolmk << 10;
+
+	uma_memory_size = (uint64_t)fbufk << 10;
+	uma_memory_base = (uint64_t)tolmk << 10;
+
+	if (uma_memory_size > UINT32_MAX)
+		die("uma_memory_size %llu exceeds 32-bit address range\n", uma_memory_size);
+
+	if (uma_memory_base > UINT32_MAX)
+		die("uma_memory_base %llu exceeds 32-bit address range\n", uma_memory_base);

//uma_resource(dev, idx++, uma_memory_base>>10, uma_memory_size>>10);