[PATCH 0/2] Fix command line underflows and out-of-bounds write

[PATCH 0/2] Fix command line underflows and out-of-bounds write

[Ryan Cohen]

Hello everyone,

I was messing around in the GRUB command line and I found two related
integer underflows that occur on all platforms. I also found an
out-of-bounds write that occurs only on i386 systems using the VGA text
terminal. This out-of-bounds write is caused by one of the underflows,
but I've included 2 patches so that each bug is fixed.

This is my first patch submission for GRUB, so please let me know if
there is anything I should change or fix. I really appreciate feedback!

Thanks to Daniel Kiper for helping me figure out the process of
submitting a patch. :)

Ryan Cohen (2):
  vga_text: Prevent out-of-bounds writes to VGA text buffer
  cmdline: Fix two related integer underflows

 grub-core/normal/cmdline.c        | 7 ++++++-
 grub-core/term/i386/pc/vga_text.c | 3 ++-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.38.1

[PATCH 1/2] vga_text: Prevent out-of-bounds writes to VGA text buffer

[Ryan Cohen]

Coordinates passed to screen_write_char() did not have any checks to
ensure they are not out-of-bounds. This adds an if statement to prevent
out-of-bounds writes to the VGA text buffer.

Signed-off-by: Ryan Cohen <rcohenprogramming@gmail.com>
---
 grub-core/term/i386/pc/vga_text.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/term/i386/pc/vga_text.c b/grub-core/term/i386/pc/vga_text.c
index 669d06fad..b88fa9d2e 100644
--- a/grub-core/term/i386/pc/vga_text.c
+++ b/grub-core/term/i386/pc/vga_text.c
@@ -63,7 +63,8 @@ static grub_uint8_t cur_color = 0x7;
 static void
 screen_write_char (int x, int y, short c)
 {
-  VGA_TEXT_SCREEN[y * COLS + x] = grub_cpu_to_le16 (c);
+  if (x < COLS && y < ROWS && x >= 0 && y >= 0)
+    VGA_TEXT_SCREEN[y * COLS + x] = grub_cpu_to_le16 (c);
 }
 
 static short
-- 
2.38.1

[PATCH 2/2] cmdline: Fix two related integer underflows

[Ryan Cohen]

An unchecked decrement operation in cl_print() would cause a few
integers to underflow. Where an output terminal's state is stored in
cl_term, the values cl_term->ystart and cl_term->pos.y both underflow.

This can be replicated with the following steps:

1. Get to the GRUB command line
2. Hold down the `d` key (or any key that enters a visible character)
   until it fills the entire row
3. Press `HOME` and then press `CTRL-k`. This will clear every
   character entered in step 2
4. Continuously press `CTRL-y` until the terminal scrolls the original
   prompt ("grub> ") passed the terminal's top row. Now, no prompt
   should be visible. This step causes cl_term->ystart to underflow
5. Press `HOME` and then `d` (or any visible character). This can have
   different visual effects for different systems, but it will always
   cause cl_term->pos.y to underflow

On BIOS systems, these underflows cause the output terminal to
completely stop displaying anything. Characters can still be
entered and commands can be run, but nothing will display on the
terminal. From here, you can only get the display working by running a
command to switch the current output terminal to a different type:

terminal_output <OTHER_TERMINAL>

On UEFI systems, these replication steps do not break the output
terminal. Until you press `ENTER`, the cursor stops responding to input,
but you can press `ENTER` after step 5 and the command line will
work properly again. This patch is mostly important for BIOS systems
where the output terminal is rendered unusable after the underflows
occur.

This patch adds two checks, one for each variable. It ensures that
cl_term->ystart does not decrement passed 0. It also ensures that
cl_term->pos.y does not get set passed the terminal's bottom row.

When the previously listed replication steps are followed with this
patch, the terminal's cursor will be set to the top row and the command
line is still usable, even on BIOS systems.

Signed-off-by: Ryan Cohen <rcohenprogramming@gmail.com>
---
 grub-core/normal/cmdline.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/grub-core/normal/cmdline.c b/grub-core/normal/cmdline.c
index 61f098244..781f6bc6f 100644
--- a/grub-core/normal/cmdline.c
+++ b/grub-core/normal/cmdline.c
@@ -219,6 +219,8 @@ cl_set_pos (struct cmdline_term *cl_term, grub_size_t lpos)
   cl_term->pos.x = (cl_term->prompt_len + lpos) % cl_term->width;
   cl_term->pos.y = cl_term->ystart
     + (cl_term->prompt_len + lpos) / cl_term->width;
+  if (cl_term->pos.y >= cl_term->height)
+    cl_term->pos.y = cl_term->height - 1;
   grub_term_gotoxy (cl_term->term, cl_term->pos);
 }
 
@@ -248,7 +250,10 @@ cl_print (struct cmdline_term *cl_term, grub_uint32_t c,
 	{
 	  cl_term->pos.x = 0;
 	  if (cl_term->pos.y >= (unsigned) (cl_term->height - 1))
-	    cl_term->ystart--;
+    {
+      if (cl_term->ystart > 0)
+        cl_term->ystart--;
+    }
 	  else
 	    cl_term->pos.y++;
 	  grub_putcode ('\n', cl_term->term);
-- 
2.38.1

Re: [PATCH 0/2] Fix command line underflows and out-of-bounds write

[Daniel Kiper]

On Sat, Nov 26, 2022 at 05:22:50PM -0500, Ryan Cohen wrote:
> Hello everyone,
>
> I was messing around in the GRUB command line and I found two related
> integer underflows that occur on all platforms. I also found an
> out-of-bounds write that occurs only on i386 systems using the VGA text
> terminal. This out-of-bounds write is caused by one of the underflows,
> but I've included 2 patches so that each bug is fixed.
>
> This is my first patch submission for GRUB, so please let me know if
> there is anything I should change or fix. I really appreciate feedback!
>
> Thanks to Daniel Kiper for helping me figure out the process of
> submitting a patch. :)
>
> Ryan Cohen (2):
>   vga_text: Prevent out-of-bounds writes to VGA text buffer
>   cmdline: Fix two related integer underflows

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> for both patches...

Thank you for finding and fixing these issues!

Daniel