[U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol

[U-Boot] [PATCH 0/2] efi_loader: fixes for Simple Output Protocol

[Heinrich Schuchardt]

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

[U-Boot] [PATCH 1/2] efi_loader: cursor positioning

[Heinrich Schuchardt]

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

[U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode

[Heinrich Schuchardt]

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

[U-Boot] [PATCH 1/2] efi_loader: cursor positioning

[Alexander Graf]

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

[U-Boot] [PATCH 2/2] efi_loader: do not set invalid screen mode

[Alexander Graf]

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

[U-Boot] [PATCH 1/2] efi_loader: cursor positioning

[Heinrich Schuchardt]

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
> 
> 
> 

[U-Boot] [PATCH 1/2] efi_loader: cursor positioning

[Alexander Graf]

> 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