Hello Julius Werner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38536
to review the following change.
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
libpayload/corebootfb: Fix character buffer relocation
The `chars` pointer references the heap which is part of the payload and relocated along with it. So calling phys_to_virt() on it was always wrong; and the virt_to_phys() at its initialization was a no-op anyway, when the console was brought up before relocation.
While we are at it, add a null-pointer check.
Change-Id: Ic03150f0bcd14a6ec6bf514dffe2b9153d5a6d2a Signed-off-by: Nico Huber nico.huber@secunet.com --- M payloads/libpayload/drivers/video/corebootfb.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38536/1
diff --git a/payloads/libpayload/drivers/video/corebootfb.c b/payloads/libpayload/drivers/video/corebootfb.c index b5ad1a5..11397ba 100644 --- a/payloads/libpayload/drivers/video/corebootfb.c +++ b/payloads/libpayload/drivers/video/corebootfb.c @@ -64,11 +64,11 @@ /* Addresses for the various components */ static unsigned long fbinfo; static unsigned long fbaddr; -static unsigned long chars; +static unsigned short *chars;
#define FI ((struct cb_framebuffer *) phys_to_virt(fbinfo)) #define FB ((unsigned char *) phys_to_virt(fbaddr)) -#define CHARS ((unsigned short *) phys_to_virt(chars)) +#define CHARS (chars)
static void corebootfb_scroll_up(void) { @@ -243,9 +243,10 @@ coreboot_video_console.columns = FI->x_resolution / font_width; coreboot_video_console.rows = FI->y_resolution / font_height;
- /* See setting of fbinfo above. */ - chars = virt_to_phys(malloc(coreboot_video_console.rows * - coreboot_video_console.columns * 2)); + chars = malloc(coreboot_video_console.rows * + coreboot_video_console.columns * 2); + if (!chars) + return -1;
// clear boot splash screen if there is one. corebootfb_clear();
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1: Code-Review+1
Has this been tested?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
Has this been tested?
Yes, at the end of the patch train only, though.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
Please mention that it fixes non-bootable payloads?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
Please mention that it fixes non-bootable payloads?
It doesn't. It may fix something, however. There's a potential corruption of unreserved memory. Which may lead to more subtle errors or no errors at all.
In case of FILO, it kept writing to the old heap address after relocation. If the kernel to be loaded resides at that address, *boom*, but otherwise /whatever/.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1: Code-Review+2
Ack
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 71: #define CHARS (chars) nit: Can we just use 'chars' in the code now?
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 246: chars = malloc(coreboot_video_console.rows * So you're saying this call happens before relocation, right? Then wouldn't 'chars' hold the physical address of the allocated buffer? And then later when the code is accessing it, the buffer will be mapped at a different address, so you'd need a phys_to_virt() again? Maybe the virt_to_phys() here was essentially a no-op (although I don't think that's necessarily true when CONFIG_LP_SKIP_CONSOLE_INIT is set and the payload does it at a later time), but the phys_to_virt() still had a purpose?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
(2 comments)
Somehow thought, I answered the comments already. Sorry for the delay.
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 71: #define CHARS (chars)
nit: Can we just use 'chars' in the code now?
Yeah, shall I push a follow-up?
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 246: chars = malloc(coreboot_video_console.rows *
So you're saying this call happens before relocation, right? Then wouldn't 'chars' hold the physical […]
Before relocation, it would hold the physical == virtual location. The heap is relocated along with the payload, though, so the virtual address stays valid. That means without the phys_to_virt() above, we'll access a different physical location, but that's just what we want.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 247: virt_to_phys I added this btw. Must have assumed that the phys_to_virt() is correct, then this virt_to_phys() would have been necessary for late initializations (as Julius also pointed out).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 246: chars = malloc(coreboot_video_console.rows *
Before relocation, it would hold the physical == virtual location. The heap […]
Right, sorry, I guess I didn't really understand how this relocation thing works. Makes sense.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/corebootfb.c:
https://review.coreboot.org/c/coreboot/+/38536/1/payloads/libpayload/drivers... PS1, Line 247: virt_to_phys
I added this btw. Must have assumed that the phys_to_virt() is correct, then […]
Ack
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
libpayload/corebootfb: Fix character buffer relocation
The `chars` pointer references the heap which is part of the payload and relocated along with it. So calling phys_to_virt() on it was always wrong; and the virt_to_phys() at its initialization was a no-op anyway, when the console was brought up before relocation.
While we are at it, add a null-pointer check.
Change-Id: Ic03150f0bcd14a6ec6bf514dffe2b9153d5a6d2a Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38536 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/video/corebootfb.c 1 file changed, 6 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/video/corebootfb.c b/payloads/libpayload/drivers/video/corebootfb.c index b5ad1a5..11397ba 100644 --- a/payloads/libpayload/drivers/video/corebootfb.c +++ b/payloads/libpayload/drivers/video/corebootfb.c @@ -64,11 +64,11 @@ /* Addresses for the various components */ static unsigned long fbinfo; static unsigned long fbaddr; -static unsigned long chars; +static unsigned short *chars;
#define FI ((struct cb_framebuffer *) phys_to_virt(fbinfo)) #define FB ((unsigned char *) phys_to_virt(fbaddr)) -#define CHARS ((unsigned short *) phys_to_virt(chars)) +#define CHARS (chars)
static void corebootfb_scroll_up(void) { @@ -243,9 +243,10 @@ coreboot_video_console.columns = FI->x_resolution / font_width; coreboot_video_console.rows = FI->y_resolution / font_height;
- /* See setting of fbinfo above. */ - chars = virt_to_phys(malloc(coreboot_video_console.rows * - coreboot_video_console.columns * 2)); + chars = malloc(coreboot_video_console.rows * + coreboot_video_console.columns * 2); + if (!chars) + return -1;
// clear boot splash screen if there is one. corebootfb_clear();
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38536 )
Change subject: libpayload/corebootfb: Fix character buffer relocation ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/652 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/651 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/650
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)