[U-Boot] [PATCH v2 1/2] efi_loader: Fix serial console size detection

[U-Boot] [PATCH v2 1/2] efi_loader: Fix serial console size detection

[matthias.bgg at kernel.org]

From: Matthias Brugger <mbrugger@suse.com>

Function term_read_reply tries to read from the serial console until
the end_char was read. This can hang forever if we are, for some reason,
not be able to read the full response (e.g. serial buffer too small,
frame error). This patch moves the timeout detection into
term_read_reply to assure we will make progress.

Fixes: 6bb591f704 ("efi_loader: query serial console size reliably")
Signed-off-by: Matthias Brugger <mbrugger@suse.com>

---

Changes in v2:
- move timeout into term_get_char

 lib/efi_loader/efi_console.c | 60 ++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index 66c33a551d..1133674faf 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -62,6 +62,21 @@ static struct simple_text_output_mode efi_con_mode = {
 	.cursor_visible = 1,
 };
 
+static int term_get_char(char *c)
+{
+	u64 timeout;
+
+	/* Wait up to 100 ms for a character */
+	timeout = timer_get_us() + 100000;
+
+	while (!tstc())
+		if (timer_get_us() > timeout)
+			return 1;
+
+	*c = getc();
+	return 0;
+}
+
 /*
  * Receive and parse a reply from the terminal.
  *
@@ -75,31 +90,31 @@ static int term_read_reply(int *n, int num, char end_char)
 	char c;
 	int i = 0;
 
-	c = getc();
-	if (c != cESC)
+	if (term_get_char(&c) || c != cESC)
 		return -1;
-	c = getc();
-	if (c != '[')
+
+	if (term_get_char(&c) || c != '[')
 		return -1;
 
 	n[0] = 0;
 	while (1) {
-		c = getc();
-		if (c == ';') {
-			i++;
-			if (i >= num)
+		if (!term_get_char(&c)) {
+			if (c == ';') {
+				i++;
+				if (i >= num)
+					return -1;
+				n[i] = 0;
+				continue;
+			} else if (c == end_char) {
+				break;
+			} else if (c > '9' || c < '0') {
 				return -1;
-			n[i] = 0;
-			continue;
-		} else if (c == end_char) {
-			break;
-		} else if (c > '9' || c < '0') {
-			return -1;
-		}
+			}
 
-		/* Read one more decimal position */
-		n[i] *= 10;
-		n[i] += c - '0';
+			/* Read one more decimal position */
+			n[i] *= 10;
+			n[i] += c - '0';
+		}
 	}
 	if (i != num - 1)
 		return -1;
@@ -196,7 +211,6 @@ static int query_console_serial(int *rows, int *cols)
 {
 	int ret = 0;
 	int n[2];
-	u64 timeout;
 
 	/* Empty input buffer */
 	while (tstc())
@@ -216,14 +230,6 @@ static int query_console_serial(int *rows, int *cols)
 	       ESC "[999;999H"	/* Move to bottom right corner */
 	       ESC "[6n");	/* Query cursor position */
 
-	/* Allow up to one second for a response */
-	timeout = timer_get_us() + 1000000;
-	while (!tstc())
-		if (timer_get_us() > timeout) {
-			ret = -1;
-			goto out;
-		}
-
 	/* Read {rows,cols} */
 	if (term_read_reply(n, 2, 'R')) {
 		ret = 1;
-- 
2.20.1

[U-Boot] [PATCH v2 2/2] efi_loader: Fix possible starving in efi_cin_read_key

[matthias.bgg at kernel.org]

From: Matthias Brugger <mbrugger@suse.com>

The function efi_cin_read_key can be affected by a loss of
a character which will make u-boot to wait forever.
Fix this by calling term_get_char instead.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---

Changes in v2: None

 lib/efi_loader/efi_console.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index 1133674faf..558aaed109 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -493,13 +493,14 @@ static int analyze_modifiers(struct efi_key_state *key_state)
 {
 	int c, mod = 0, ret = 0;
 
-	c = getc();
+	if (!term_get_char(&c))
+		goto out;
 
 	if (c != ';') {
 		ret = c;
 		if (c == '~')
 			goto out;
-		c = getc();
+		term_get_char(&c);
 	}
 	for (;;) {
 		switch (c) {
@@ -508,7 +509,7 @@ static int analyze_modifiers(struct efi_key_state *key_state)
 			mod += c - '0';
 		/* fall through */
 		case ';':
-			c = getc();
+			term_get_char(&c);
 			break;
 		default:
 			goto out;
@@ -551,7 +552,9 @@ static efi_status_t efi_cin_read_key(struct efi_key_data *key)
 		 * Xterm Control Sequences
 		 * https://www.xfree86.org/4.8.0/ctlseqs.html
 		 */
-		ch = getc();
+		if (!term_get_char(&ch))
+			return EFI_NOT_READY;
+
 		switch (ch) {
 		case cESC: /* ESC */
 			pressed_key.scan_code = 23;
@@ -561,12 +564,15 @@ static efi_status_t efi_cin_read_key(struct efi_key_data *key)
 			/* consider modifiers */
 			if (ch < 'P') {
 				set_shift_mask(ch - '0', &key->key_state);
-				ch = getc();
+				if (!term_get_char(&ch))
+					return EFI_NOT_READY;
 			}
 			pressed_key.scan_code = ch - 'P' + 11;
 			break;
 		case '[':
-			ch = getc();
+			if (!term_get_char(&ch))
+				return EFI_NOT_READY;
+
 			switch (ch) {
 			case 'A'...'D': /* up, down right, left */
 				pressed_key.scan_code = ch - 'A' + 1;
-- 
2.20.1

[U-Boot] [PATCH v2 1/2] efi_loader: Fix serial console size detection

[Heinrich Schuchardt]

On 3/5/19 12:50 PM, matthias.bgg at kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> Function term_read_reply tries to read from the serial console until
> the end_char was read. This can hang forever if we are, for some reason,
> not be able to read the full response (e.g. serial buffer too small,
> frame error). This patch moves the timeout detection into
> term_read_reply to assure we will make progress.
> 
> Fixes: 6bb591f704 ("efi_loader: query serial console size reliably")
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH v2 1/2] efi_loader: Fix serial console size detection

[Heinrich Schuchardt]

On 3/5/19 12:50 PM, matthias.bgg at kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> Function term_read_reply tries to read from the serial console until
> the end_char was read. This can hang forever if we are, for some reason,
> not be able to read the full response (e.g. serial buffer too small,
> frame error). This patch moves the timeout detection into
> term_read_reply to assure we will make progress.
> 
> Fixes: 6bb591f704 ("efi_loader: query serial console size reliably")
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> 
> ---
> 
> Changes in v2:
> - move timeout into term_get_char
> 
>  lib/efi_loader/efi_console.c | 60 ++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 66c33a551d..1133674faf 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -62,6 +62,21 @@ static struct simple_text_output_mode efi_con_mode = {
>  	.cursor_visible = 1,
>  };
>  
> +static int term_get_char(char *c)
> +{
> +	u64 timeout;
> +
> +	/* Wait up to 100 ms for a character */
> +	timeout = timer_get_us() + 100000;
> +
> +	while (!tstc())
> +		if (timer_get_us() > timeout)
> +			return 1;
> +
> +	*c = getc();
> +	return 0;
> +}
> +
>  /*
>   * Receive and parse a reply from the terminal.
>   *
> @@ -75,31 +90,31 @@ static int term_read_reply(int *n, int num, char end_char)
>  	char c;
>  	int i = 0;
>  
> -	c = getc();
> -	if (c != cESC)
> +	if (term_get_char(&c) || c != cESC)
>  		return -1;
> -	c = getc();
> -	if (c != '[')
> +
> +	if (term_get_char(&c) || c != '[')
>  		return -1;
>  
>  	n[0] = 0;
>  	while (1) {
> -		c = getc();
> -		if (c == ';') {
> -			i++;
> -			if (i >= num)
> +		if (!term_get_char(&c)) {
> +			if (c == ';') {
> +				i++;
> +				if (i >= num)
> +					return -1;
> +				n[i] = 0;
> +				continue;
> +			} else if (c == end_char) {
> +				break;
> +			} else if (c > '9' || c < '0') {
>  				return -1;
> -			n[i] = 0;
> -			continue;
> -		} else if (c == end_char) {
> -			break;
> -		} else if (c > '9' || c < '0') {
> -			return -1;
> -		}
> +			}
>  
> -		/* Read one more decimal position */
> -		n[i] *= 10;
> -		n[i] += c - '0';
> +			/* Read one more decimal position */
> +			n[i] *= 10;
> +			n[i] += c - '0';
> +		}

If no character is received we should throw an error:

	} else {
		return -1;

Best regards

Heinrich

>  	}
>  	if (i != num - 1)
>  		return -1;
> @@ -196,7 +211,6 @@ static int query_console_serial(int *rows, int *cols)
>  {
>  	int ret = 0;
>  	int n[2];
> -	u64 timeout;
>  
>  	/* Empty input buffer */
>  	while (tstc())
> @@ -216,14 +230,6 @@ static int query_console_serial(int *rows, int *cols)
>  	       ESC "[999;999H"	/* Move to bottom right corner */
>  	       ESC "[6n");	/* Query cursor position */
>  
> -	/* Allow up to one second for a response */
> -	timeout = timer_get_us() + 1000000;
> -	while (!tstc())
> -		if (timer_get_us() > timeout) {
> -			ret = -1;
> -			goto out;
> -		}
> -
>  	/* Read {rows,cols} */
>  	if (term_read_reply(n, 2, 'R')) {
>  		ret = 1;
> 

[U-Boot] [PATCH v2 2/2] efi_loader: Fix possible starving in efi_cin_read_key

[Heinrich Schuchardt]

On 3/5/19 12:50 PM, matthias.bgg at kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> The function efi_cin_read_key can be affected by a loss of
> a character which will make u-boot to wait forever.
> Fix this by calling term_get_char instead.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>

You can test EFI keyboard input with:

=> setenv efi_selftest extended text input
=> bootefi selftest

Function keys, arrow keys, PG UP, PG DN, etc. do not work with this
patch in place.

> ---
> 
> Changes in v2: None
> 
>  lib/efi_loader/efi_console.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 1133674faf..558aaed109 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -493,13 +493,14 @@ static int analyze_modifiers(struct efi_key_state *key_state)
>  {
>  	int c, mod = 0, ret = 0;
>  
> -	c = getc();
> +	if (!term_get_char(&c))
> +		goto out;
>  
>  	if (c != ';') {
>  		ret = c;
>  		if (c == '~')
>  			goto out;
> -		c = getc();
> +		term_get_char(&c);

lib/efi_loader/efi_console.c:496:21: warning: passing argument 1 of
‘term_get_char’ from incompatible pointer type
[-Wincompatible-pointer-types]
  if (!term_get_char(&c))
                     ^~
lib/efi_loader/efi_console.c:65:32: note: expected ‘char *’ but argument
is of type ‘int *’
 static int term_get_char(char *c)

The problem repeats throughout the patch.

Best thing is to define the argument of term_get_char() as s32 in the
first patch of the series.

>  	}
>  	for (;;) {
>  		switch (c) {
> @@ -508,7 +509,7 @@ static int analyze_modifiers(struct efi_key_state *key_state)
>  			mod += c - '0';
>  		/* fall through */
>  		case ';':
> -			c = getc();
> +			term_get_char(&c);
>  			break;
>  		default:
>  			goto out;
> @@ -551,7 +552,9 @@ static efi_status_t efi_cin_read_key(struct efi_key_data *key)
>  		 * Xterm Control Sequences
>  		 * https://www.xfree86.org/4.8.0/ctlseqs.html
>  		 */
> -		ch = getc();
> +		if (!term_get_char(&ch))
> +			return EFI_NOT_READY;
> +
>  		switch (ch) {
>  		case cESC: /* ESC */
>  			pressed_key.scan_code = 23;
> @@ -561,12 +564,15 @@ static efi_status_t efi_cin_read_key(struct efi_key_data *key)

You missed a getc() in line 560.

Best regards

Heinrich

>  			/* consider modifiers */
>  			if (ch < 'P') {
>  				set_shift_mask(ch - '0', &key->key_state);
> -				ch = getc();
> +				if (!term_get_char(&ch))
> +					return EFI_NOT_READY;
>  			}
>  			pressed_key.scan_code = ch - 'P' + 11;
>  			break;
>  		case '[':
> -			ch = getc();
> +			if (!term_get_char(&ch))
> +				return EFI_NOT_READY;
> +
>  			switch (ch) {
>  			case 'A'...'D': /* up, down right, left */
>  				pressed_key.scan_code = ch - 'A' + 1;
>