[U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol
Fix errors in the simple output protocol which may lead to providing incorrect values for columns or rows.
Allowing to set an illegal screen mode has led to an illegal memory access in the UEFI SCT.
For reference:
As the illegal memory access led to QEMU stopping the following patch for Linux has been proposed. With the patch QEMU does not stop but hands the error back to U-Boot which than outputs the relative position in the loaded UEFI binary or in U-Boot (in this case SetMem16() called by AppendStringToHistory() of EDK2's ConsoleLogger).
KVM: inject data abort if instruction cannot be decoded https://lkml.org/lkml/2019/9/4/1488
Heinrich Schuchardt (2): efi_loader: cursor positioning efi_loader: do not set invalid screen mode
lib/efi_loader/efi_console.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
-- 2.23.0.rc1
When backspacing in column 0 do no set the column index to ULONG_MAX. Ensure that the row number is not set to ULONG_MAX even if the row count is advertised as 0. Ignore control characters other the 0x08, 0x0a, 0x0d when updating the column.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_loader/efi_console.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d4765afb98..d5222c46b4 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -156,13 +156,14 @@ static efi_status_t EFIAPI efi_cout_output_string( * Update the cursor position. * * The UEFI spec provides advance rules for U+0000, U+0008, U+000A, - * and U000D. All other characters, including control characters - * U+0007 (BEL) and U+0009 (TAB), have to increase the column by one. + * and U000D. All other control characters are ignored. Any non-control + * character increase the column by one. */ for (p = string; *p; ++p) { switch (*p) { case '\b': /* U+0008, backspace */ - con->cursor_column = max(0, con->cursor_column - 1); + if (con->cursor_column) + con->cursor_column--; break; case '\n': /* U+000A, newline */ con->cursor_column = 0; @@ -178,13 +179,16 @@ static efi_status_t EFIAPI efi_cout_output_string( */ break; default: - con->cursor_column++; + if (*p > 0x1f) + con->cursor_column++; break; } if (con->cursor_column >= mode->columns) { con->cursor_column = 0; con->cursor_row++; } + if (con->cursor_row >= mode->rows && con->cursor_row) + con->cursor_row--; con->cursor_row = min(con->cursor_row, (s32)mode->rows - 1); }
-- 2.23.0.rc1
On 05.09.19 10:06, Heinrich Schuchardt wrote:
When backspacing in column 0 do no set the column index to ULONG_MAX. Ensure that the row number is not set to ULONG_MAX even if the row count is advertised as 0. Ignore control characters other the 0x08, 0x0a, 0x0d when updating the column.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
lib/efi_loader/efi_console.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d4765afb98..d5222c46b4 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -156,13 +156,14 @@ static efi_status_t EFIAPI efi_cout_output_string( * Update the cursor position. * * The UEFI spec provides advance rules for U+0000, U+0008, U+000A,
* and U000D. All other characters, including control characters* U+0007 (BEL) and U+0009 (TAB), have to increase the column by one.
* and U000D. All other control characters are ignored. Any non-control* character increase the column by one.
con->cursor_column = max(0, con->cursor_column - 1);
if (con->cursor_column)con->cursor_column--; break;@@ -178,13 +179,16 @@ static efi_status_t EFIAPI efi_cout_output_string( */ break; default:
con->cursor_column++;
if (*p > 0x1f)
What is the 0x1f here? I know, control character, but it's not obvious. Probably wants either a comment or a define.
con->cursor_column++; break;if (con->cursor_row >= mode->rows && con->cursor_row)con->cursor_row--;
I don't understand this statement. When is the cursor_row >= mode->rows? Is this just to offset an incorrect cursor_row from the line above? Can't we just fold it in there?
Alex
On 9/5/19 10:21 PM, Alexander Graf wrote:
On 05.09.19 10:06, Heinrich Schuchardt wrote:
When backspacing in column 0 do no set the column index to ULONG_MAX. Ensure that the row number is not set to ULONG_MAX even if the row count is advertised as 0. Ignore control characters other the 0x08, 0x0a, 0x0d when updating the column.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
lib/efi_loader/efi_console.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d4765afb98..d5222c46b4 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -156,13 +156,14 @@ static efi_status_t EFIAPI efi_cout_output_string( * Update the cursor position. * * The UEFI spec provides advance rules for U+0000, U+0008, U+000A, - * and U000D. All other characters, including control characters - * U+0007 (BEL) and U+0009 (TAB), have to increase the column by one. + * and U000D. All other control characters are ignored. Any non-control + * character increase the column by one. */ for (p = string; *p; ++p) { switch (*p) { case '\b': /* U+0008, backspace */ - con->cursor_column = max(0, con->cursor_column - 1); + if (con->cursor_column) + con->cursor_column--; break; case '\n': /* U+000A, newline */ con->cursor_column = 0; @@ -178,13 +179,16 @@ static efi_status_t EFIAPI efi_cout_output_string( */ break; default: - con->cursor_column++; + if (*p > 0x1f)
What is the 0x1f here? I know, control character, but it's not obvious. Probably wants either a comment or a define.
I will add a comment
+ con->cursor_column++; break; } if (con->cursor_column >= mode->columns) { con->cursor_column = 0; con->cursor_row++; } + if (con->cursor_row >= mode->rows && con->cursor_row) + con->cursor_row--;
I don't understand this statement. When is the cursor_row >= mode->rows? Is this just to offset an incorrect cursor_row from the line above? Can't we just fold it in there?
The row is incremented either by line feed or by exceeding the last column.
When we exceed the row limit the terminal will take care of scrolling. We have to compensate for the scrolling by decrementing the row.
Best regards
Heinrich
Alex
Am 05.09.2019 um 22:35 schrieb Heinrich Schuchardt xypron.glpk@gmx.de:
On 9/5/19 10:21 PM, Alexander Graf wrote:
On 05.09.19 10:06, Heinrich Schuchardt wrote: When backspacing in column 0 do no set the column index to ULONG_MAX. Ensure that the row number is not set to ULONG_MAX even if the row count is advertised as 0. Ignore control characters other the 0x08, 0x0a, 0x0d when updating the column.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
lib/efi_loader/efi_console.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d4765afb98..d5222c46b4 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -156,13 +156,14 @@ static efi_status_t EFIAPI efi_cout_output_string( * Update the cursor position. * * The UEFI spec provides advance rules for U+0000, U+0008, U+000A,
* and U000D. All other characters, including control characters* U+0007 (BEL) and U+0009 (TAB), have to increase the column by one.
* and U000D. All other control characters are ignored. Any non-control* character increase the column by one. */ for (p = string; *p; ++p) { switch (*p) { case '\b': /* U+0008, backspace */
con->cursor_column = max(0, con->cursor_column - 1);
if (con->cursor_column)con->cursor_column--; break; case '\n': /* U+000A, newline */ con->cursor_column = 0;@@ -178,13 +179,16 @@ static efi_status_t EFIAPI efi_cout_output_string( */ break; default:
con->cursor_column++;
if (*p > 0x1f)What is the 0x1f here? I know, control character, but it's not obvious. Probably wants either a comment or a define.
I will add a comment
con->cursor_column++; break; } if (con->cursor_column >= mode->columns) { con->cursor_column = 0; con->cursor_row++; }if (con->cursor_row >= mode->rows && con->cursor_row)con->cursor_row--;I don't understand this statement. When is the cursor_row >= mode->rows? Is this just to offset an incorrect cursor_row from the line above? Can't we just fold it in there?
The row is incremented either by line feed or by exceeding the last column.
When we exceed the row limit the terminal will take care of scrolling. We have to compensate for the scrolling by decrementing the row.
Please indicate either through a variable or a comment that this compensates for already executed scrolling.
Alex
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.SetMode() should return EFI_UNDEFINED if a screen mode is not available.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_loader/efi_console.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d5222c46b4..540a009145 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -375,6 +375,10 @@ static efi_status_t EFIAPI efi_cout_set_mode(
if (mode_number >= efi_con_mode.max_mode) return EFI_EXIT(EFI_UNSUPPORTED); + + if (!efi_cout_modes[mode_number].present) + return EFI_EXIT(EFI_UNSUPPORTED); + efi_con_mode.mode = mode_number; EFI_CALL(efi_cout_clear_screen(this));
-- 2.23.0.rc1
On 05.09.19 10:06, Heinrich Schuchardt wrote:
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.SetMode() should return EFI_UNDEFINED if a screen mode is not available.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Reviewed-by: Alexander Graf agraf@csgraf.de
Alex
participants (2)
-
Alexander Graf -
Heinrich Schuchardt