* [U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol @ 2019-09-05 8:06 Heinrich Schuchardt 2019-09-05 8:06 ` [U-Boot] [PATCH 1/2] efi_loader: cursor positioning Heinrich Schuchardt 2019-09-05 8:06 ` [U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode Heinrich Schuchardt 0 siblings, 2 replies; 7+ messages in thread From: Heinrich Schuchardt @ 2019-09-05 8:06 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: cursor positioning 2019-09-05 8:06 [U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol Heinrich Schuchardt @ 2019-09-05 8:06 ` Heinrich Schuchardt 2019-09-05 20:21 ` Alexander Graf 2019-09-05 8:06 ` [U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode Heinrich Schuchardt 1 sibling, 1 reply; 7+ messages in thread From: Heinrich Schuchardt @ 2019-09-05 8:06 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: cursor positioning 2019-09-05 8:06 ` [U-Boot] [PATCH 1/2] efi_loader: cursor positioning Heinrich Schuchardt @ 2019-09-05 20:21 ` Alexander Graf 2019-09-05 20:35 ` Heinrich Schuchardt 0 siblings, 1 reply; 7+ messages in thread From: Alexander Graf @ 2019-09-05 20:21 UTC (permalink / raw) To: u-boot 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. > + 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? Alex ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: cursor positioning 2019-09-05 20:21 ` Alexander Graf @ 2019-09-05 20:35 ` Heinrich Schuchardt 2019-09-06 6:18 ` Alexander Graf 0 siblings, 1 reply; 7+ messages in thread From: Heinrich Schuchardt @ 2019-09-05 20:35 UTC (permalink / raw) To: u-boot 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: cursor positioning 2019-09-05 20:35 ` Heinrich Schuchardt @ 2019-09-06 6:18 ` Alexander Graf 0 siblings, 0 replies; 7+ messages in thread From: Alexander Graf @ 2019-09-06 6:18 UTC (permalink / raw) To: u-boot > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode 2019-09-05 8:06 [U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol Heinrich Schuchardt 2019-09-05 8:06 ` [U-Boot] [PATCH 1/2] efi_loader: cursor positioning Heinrich Schuchardt @ 2019-09-05 8:06 ` Heinrich Schuchardt 2019-09-05 20:26 ` Alexander Graf 1 sibling, 1 reply; 7+ messages in thread From: Heinrich Schuchardt @ 2019-09-05 8:06 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode 2019-09-05 8:06 ` [U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode Heinrich Schuchardt @ 2019-09-05 20:26 ` Alexander Graf 0 siblings, 0 replies; 7+ messages in thread From: Alexander Graf @ 2019-09-05 20:26 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-06 6:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-05 8:06 [U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol Heinrich Schuchardt 2019-09-05 8:06 ` [U-Boot] [PATCH 1/2] efi_loader: cursor positioning Heinrich Schuchardt 2019-09-05 20:21 ` Alexander Graf 2019-09-05 20:35 ` Heinrich Schuchardt 2019-09-06 6:18 ` Alexander Graf 2019-09-05 8:06 ` [U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode Heinrich Schuchardt 2019-09-05 20:26 ` Alexander Graf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.