From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date: Tue, 5 Mar 2019 19:44:35 +0100
Subject: [U-Boot] [PATCH v2 2/2] efi_loader: Fix possible starving in
 efi_cin_read_key
In-Reply-To: <20190305115019.3581-2-matthias.bgg@kernel.org>
References: <20190305115019.3581-1-matthias.bgg@kernel.org>
 <20190305115019.3581-2-matthias.bgg@kernel.org>
Message-ID: <2a6c2a65-9502-4114-aec6-89bfd10f1f74@gmx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

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