Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Assert framebuffer size is within limits ......................................................................
nb/via/vx900: Assert framebuffer size is within limits
The framebuffer size needs to be between 8 and 512 MiB, or alternatively, its power needs to be between 3 and 9. If the power is too small, an undefined integer shift will occur in the call to pci_update_config8(), so let's do a sanity check beforehand to make sure that doesn't happen.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 8 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/1
diff --git a/src/northbridge/via/vx900/memmap.c b/src/northbridge/via/vx900/memmap.c index 0c3b7bf..ae8fb78 100644 --- a/src/northbridge/via/vx900/memmap.c +++ b/src/northbridge/via/vx900/memmap.c @@ -26,8 +26,8 @@
#define MCU PCI_DEV(0, 0, 3)
-#define CHROME_9_HD_MIN_FB_SIZE 8 -#define CHROME_9_HD_MAX_FB_SIZE 512 +#define CHROME_9_HD_MIN_FB_POW 3 /* 8 MiB */ +#define CHROME_9_HD_MAX_FB_POW 9 /* 512 MiB */
/* Helper to determine the framebuffer size */ void vx900_set_chrome9hd_fb_size(u32 size_mb) @@ -37,7 +37,7 @@ int i;
/* The minimum framebuffer size is 8MB. */ - size_mb = MAX(size_mb, CHROME_9_HD_MIN_FB_SIZE); + size_mb = MAX(size_mb, (1U << CHROME_9_HD_MIN_FB_POW));
/* * We have two limitations on the maximum framebuffer size: @@ -79,10 +79,14 @@ }
/* Now round the framebuffer size to the closest power of 2 */ - u8 fb_pow = 0; + int fb_pow = 0; while (size_mb >> fb_pow) fb_pow++; fb_pow--; + + if (fb_pow < CHROME_9_HD_MIN_FB_POW || fb_pow > CHROME_9_HD_MAX_FB_POW) + die("Framebuffer power %u is out of range\n", fb_pow); + size_mb = (1 << fb_pow);
pci_update_config8(MCU, 0xa1, ~(7 << 4), (fb_pow - 2) << 4);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Assert framebuffer size is within limits ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... PS1, Line 82: int why the int?
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Assert framebuffer size is within limits ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... PS1, Line 82: int
why the int?
If size_mb is somehow zero, then fb_pow will underflow in the subtraction to 255. This technically would also be caught by the added check, but changing it to an int is less weird. I suppose I should explain that in the commit message.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Assert framebuffer size is within limits ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... PS1, Line 82: int
If size_mb is somehow zero, then fb_pow will underflow in the subtraction to 255. […]
I'd just catch the zero separately, since it's rather significant (how do you end up with size_mb = 0 other than with some serious issue in the previous code?)
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/+/34355
to look at the new patch set (#2).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Do a sanity check that size_mb is nonzero, else fb_pow will be negative, leading to an undefined integer shift. - Use an unsigned integer when calculating 1U << fb_pow. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
I don't know if this chipset's code took inspiration from italian cuisine, or if I'm just very cursed myself, but I find this function rather weird
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 84: 0 After that assert, you know the first iteration of the loop will always run. This could be a 1, or the loop could be a do-while. That way, it's easier to prove that decrementing fb_pow will not cause an underflow.
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 89: size_mb = (1U << fb_pow); I see why you would want to change this, but what if fb_pow is, e.g., 255?
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 89: size_mb = (1U << fb_pow);
I see why you would want to change this, but what if fb_pow is, e.g. […]
fb_pow is bounded by the number of bits in size_mb, so it can only be up to 31 (which then can't cause any overflow when shifting)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 81: assert(size_mb != 0); This thing needs an #include.
Alternatively, replace it with an if block covering until line 89.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 89: size_mb = (1U << fb_pow);
fb_pow is bounded by the number of bits in size_mb, so it can only be up to 31 (which then can't cau […]
Ack
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/+/34355
to look at the new patch set (#3).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Do a sanity check that size_mb is nonzero, else fb_pow will be negative, leading to an undefined integer shift. Rewrite the loop as a do-while to make clear that no underflow can happen now. - Use an unsigned integer when calculating 1U << fb_pow. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 10 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/3
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 81: assert(size_mb != 0);
This thing needs an #include. […]
Done
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 84: 0
After that assert, you know the first iteration of the loop will always run. […]
Done
https://review.coreboot.org/c/coreboot/+/34355/2/src/northbridge/via/vx900/m... PS2, Line 89: size_mb = (1U << fb_pow);
Ack
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Sorry for the nitpicking and bikeshedding :P
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0); Just realized: Even if the assert does not fire (not sure how coreboot handles them), the code will now properly round 0, so it's actually not needed.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0);
Just realized: Even if the assert does not fire (not sure how coreboot handles them), the code will […]
The rest of the code technically rounds down to the nearest power of 2, so I suppose that comment is a bit inaccurate - I'll fix it. In any event I think we should leave this assertion in, because having a zero size framebuffer is very wrong.
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/+/34355
to look at the new patch set (#4).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Do a sanity check that size_mb is nonzero, else fb_pow will be negative, leading to an undefined integer shift. Rewrite the loop as a do-while to make clear that no underflow can happen now. - Use an unsigned integer when calculating 1U << fb_pow. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0);
The rest of the code technically rounds down to the nearest power of 2, so I suppose that comment is […]
I have never bothered to check or test, but I think asserts don't get a print in console the way die() does.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0);
I have never bothered to check or test, but I think asserts don't get a print in console the way die […]
Just did: src/include/assert.h has assert() defined as a macro. It printk's a message as BIOS_EMERG, but only halts if asserts are considered fatal (Kconfig symbol). So, I think catching (size_mb == 0) with the die() on line 94 is better.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0);
Just did: src/include/assert.h has assert() defined as a macro. […]
If we do that and size_mb is 0, then it will get rounded up to 1 after the fb_pow stuff and then print an incorrect error message on line 94 (since the size is actually 0 and not 1). I think we should just change this to a die() here with the right message.
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/+/34355
to look at the new patch set (#5).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Do a sanity check that size_mb is nonzero, else fb_pow will be negative, leading to an undefined integer shift. Rewrite the loop as a do-while to make clear that no underflow can happen now. - Use an unsigned integer when calculating 1U << fb_pow. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 5: Code-Review+2
(2 comments)
Even I am tired of bikeshedding on this change
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0);
If we do that and size_mb is 0, then it will get rounded up to 1 after the fb_pow stuff and then pri […]
Ah, true.
https://review.coreboot.org/c/coreboot/+/34355/5/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/5/src/northbridge/via/vx900/m... PS5, Line 20: #include <assert.h> Can drop this now
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/+/34355
to look at the new patch set (#6).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Do a sanity check that size_mb is nonzero, else fb_pow will be negative, leading to an undefined integer shift. Rewrite the loop as a do-while to make clear that no underflow can happen now. - Use an unsigned integer when calculating 1U << fb_pow. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/6
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/3/src/northbridge/via/vx900/m... PS3, Line 83: assert(size_mb != 0);
Ah, true.
Done
https://review.coreboot.org/c/coreboot/+/34355/5/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/5/src/northbridge/via/vx900/m... PS5, Line 20: #include <assert.h>
Can drop this now
Done
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/1/src/northbridge/via/vx900/m... PS1, Line 82: int
I'd just catch the zero separately, since it's rather significant (how do you end up with size_mb = […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 6: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/6/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/6/src/northbridge/via/vx900/m... PS6, Line 85: u8 fb_pow = 0; : do { : fb_pow++; : } while (size_mb >> fb_pow); : fb_pow--; : : size_mb = (1U << fb_pow); Nit, with the die() in place, it should be the same as
size_mb = 1u << log2(size_mb);
Hello Kyösti Mälkki, Angel Pons, 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/+/34355
to look at the new patch set (#7).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Use log2() when rounding down size_mb to the closest power of 2. Do a sanity check beforehand that size_mb is nonzero, else log2() will return -1 and there will be an undefined integer shift. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 8 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/7
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/6/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/6/src/northbridge/via/vx900/m... PS6, Line 85: u8 fb_pow = 0; : do { : fb_pow++; : } while (size_mb >> fb_pow); : fb_pow--; : : size_mb = (1U << fb_pow);
Nit, with the die() in place, it should be the same as […]
It is found in src/include/lib.h
Hello Kyösti Mälkki, Angel Pons, 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/+/34355
to look at the new patch set (#8).
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Use log2() when rounding down size_mb to the closest power of 2. Do a sanity check beforehand that size_mb is nonzero, else log2() will return -1 and there will be an undefined integer shift. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 --- M src/northbridge/via/vx900/memmap.c 1 file changed, 11 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/34355/8
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34355/6/src/northbridge/via/vx900/m... File src/northbridge/via/vx900/memmap.c:
https://review.coreboot.org/c/coreboot/+/34355/6/src/northbridge/via/vx900/m... PS6, Line 85: u8 fb_pow = 0; : do { : fb_pow++; : } while (size_mb >> fb_pow); : fb_pow--; : : size_mb = (1U << fb_pow);
It is found in src/include/lib. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34355 )
Change subject: nb/via/vx900: Ensure framebuffer size is within limits ......................................................................
nb/via/vx900: Ensure framebuffer size is within limits
- Use log2() when rounding down size_mb to the closest power of 2. Do a sanity check beforehand that size_mb is nonzero, else log2() will return -1 and there will be an undefined integer shift. - The framebuffer size needs to be between 8 and 512 MiB, so check after all the calculations are done to make sure this is the case.
Change-Id: I3962e5cdc094c8da22d8dbadf16637e02fa98689 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1391086 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34355 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/northbridge/via/vx900/memmap.c 1 file changed, 11 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/northbridge/via/vx900/memmap.c b/src/northbridge/via/vx900/memmap.c index 0c3b7bf..d11dc65 100644 --- a/src/northbridge/via/vx900/memmap.c +++ b/src/northbridge/via/vx900/memmap.c @@ -21,6 +21,7 @@ #include <device/pci_ops.h> #include <cbmem.h> #include <console/console.h> +#include <lib.h>
#include "vx900.h"
@@ -78,12 +79,16 @@ size_mb = max_size_mb; }
- /* Now round the framebuffer size to the closest power of 2 */ - u8 fb_pow = 0; - while (size_mb >> fb_pow) - fb_pow++; - fb_pow--; - size_mb = (1 << fb_pow); + /* Now round down the framebuffer size to the closest power of 2 */ + if (size_mb == 0) + die("Framebuffer size is 0\n"); + + int fb_pow = log2(size_mb); + + size_mb = 1U << fb_pow; + + if (size_mb < CHROME_9_HD_MIN_FB_SIZE || size_mb > CHROME_9_HD_MAX_FB_SIZE) + die("Framebuffer size %u is out of range\n", size_mb);
pci_update_config8(MCU, 0xa1, ~(7 << 4), (fb_pow - 2) << 4); }
-
Angel Pons (Code Review) -
Jacob Garber (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)