boot: Use correct handle to find TextInputEx protocol

LibLocateProtocol will return the protocol for the first device that
supports it. But it may not actually come from the ConIn device that
we want to use here.

This should be the root cause of what was previously considered just
broken firmware. If you ask the wrong device to return some key, of
course it will never provide one.

This changes the way we handle input yet again in light of this new
knowledge and because using the correct TextInputEx with fallback to
ConIn can actually create double input in some cases.

Since we are now confident that we get the right TextInputEx, we can
use that exclusively, only falling back to ConIn if the console input
device does not support the better interface (the spec is pretty clear
that it must support it, though).

Because some firmware is broken, we still need to provide a fallback
to the previously used TextInputEx thats overrides ConIn/ConInEx if
it is functional.

1 files changed, 56 insertions, 31 deletions

diff --git a/src/boot/efi/console.c b/src/boot/efi/console.c
index 6e76745cc6..89fbd94258 100644
--- a/src/boot/efi/console.c
+++ b/src/boot/efi/console.c
@@ -23,45 +23,58 @@ static inline void EventClosep(EFI_EVENT *event) {
  * Reading input from the console sounds like an easy task to do, but thanks to broken
  * firmware it is actually a nightmare.
  *
- * There is a ConIn and TextInputEx API for this. Ideally we want to use TextInputEx,
- * because that gives us Ctrl/Alt/Shift key state information. Unfortunately, it is not
- * always available and sometimes just non-functional.
+ * There is a SimpleTextInput and SimpleTextInputEx API for this. Ideally we want to use
+ * TextInputEx, because that gives us Ctrl/Alt/Shift key state information. Unfortunately,
+ * it is not always available and sometimes just non-functional.
  *
- * On the other hand we have ConIn, where some firmware likes to just freeze on us
- * if we call ReadKeyStroke on it.
+ * On some firmware, calling ReadKeyStroke or ReadKeyStrokeEx on the default console input
+ * device will just freeze no matter what (even though it *reported* being ready).
+ * Also, multiple input protocols can be backed by the same device, but they can be out of
+ * sync. Falling back on a different protocol can end up with double input.
  *
- * Therefore, we use WaitForEvent on both ConIn and TextInputEx (if available) along
- * with a timer event. The timer ensures there is no need to call into functions
- * that might freeze on us, while still allowing us to show a timeout counter.
- */
+ * Therefore, we will perferrably use TextInputEx for ConIn if that is available. Additionally,
+ * we look for the first TextInputEx device the firmware gives us as a fallback option. It
+ * will replace ConInEx permanently if it ever reports a key press.
+ * Lastly, a timer event allows us to provide a input timeout without having to call into
+ * any input functions that can freeze on us or using a busy/stall loop. */
 EFI_STATUS console_key_read(UINT64 *key, UINT64 timeout_usec) {
-        static EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL *TextInputEx;
-        static BOOLEAN checked;
+        static EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL *conInEx = NULL, *extraInEx = NULL;
+        static BOOLEAN checked = FALSE;
         UINTN index;
-        EFI_INPUT_KEY k;
         EFI_STATUS err;
         _cleanup_(EventClosep) EFI_EVENT timer = NULL;
-        EFI_EVENT events[3] = { ST->ConIn->WaitForKey };
-        UINTN n_events = 1;
 
         assert(key);
 
         if (!checked) {
-                err = LibLocateProtocol(&SimpleTextInputExProtocol, (void **)&TextInputEx);
-                if (EFI_ERROR(err) || BS->CheckEvent(TextInputEx->WaitForKeyEx) == EFI_INVALID_PARAMETER)
+                /* Get the *first* TextInputEx device.*/
+                err = LibLocateProtocol(&SimpleTextInputExProtocol, (void **) &extraInEx);
+                if (EFI_ERROR(err) || BS->CheckEvent(extraInEx->WaitForKeyEx) == EFI_INVALID_PARAMETER)
                         /* If WaitForKeyEx fails here, the firmware pretends it talks this
                          * protocol, but it really doesn't. */
-                        TextInputEx = NULL;
+                        extraInEx = NULL;
+
+                /* Get the TextInputEx version of ST->ConIn. */
+                err = BS->HandleProtocol(ST->ConsoleInHandle, &SimpleTextInputExProtocol, (void **) &conInEx);
+                if (EFI_ERROR(err) || BS->CheckEvent(conInEx->WaitForKeyEx) == EFI_INVALID_PARAMETER)
+                        conInEx = NULL;
+
+                if (conInEx == extraInEx)
+                        extraInEx = NULL;
+
                 checked = TRUE;
         }
 
-        if (TextInputEx)
-                events[n_events++] = TextInputEx->WaitForKeyEx;
-
         err = BS->CreateEvent(EVT_TIMER, 0, NULL, NULL, &timer);
         if (EFI_ERROR(err))
                 return log_error_status_stall(err, L"Error creating timer event: %r", err);
-        events[n_events++] = timer;
+
+        EFI_EVENT events[] = {
+                timer,
+                conInEx ? conInEx->WaitForKeyEx : ST->ConIn->WaitForKey,
+                extraInEx ? extraInEx->WaitForKeyEx : NULL,
+        };
+        UINTN n_events = extraInEx ? 3 : 2;
 
         /* Watchdog rearming loop in case the user never provides us with input or some
          * broken firmware never returns from WaitForEvent. */
@@ -100,13 +113,21 @@ EFI_STATUS console_key_read(UINT64 *key, UINT64 timeout_usec) {
                 return EFI_TIMEOUT;
         }
 
-        /* TextInputEx might be ready too even if ConIn got to signal first. */
-        if (TextInputEx && !EFI_ERROR(BS->CheckEvent(TextInputEx->WaitForKeyEx))) {
+        /* If the extra input device we found returns something, always use that instead
+         * to work around broken firmware freezing on ConIn/ConInEx. */
+        if (extraInEx && !EFI_ERROR(BS->CheckEvent(extraInEx->WaitForKeyEx))) {
+                conInEx = extraInEx;
+                extraInEx = NULL;
+        }
+
+        /* Do not fall back to ConIn if we have a ConIn that supports TextInputEx.
+         * The two may be out of sync on some firmware, giving us double input. */
+        if (conInEx) {
                 EFI_KEY_DATA keydata;
                 UINT64 keypress;
                 UINT32 shift = 0;
 
-                err = TextInputEx->ReadKeyStrokeEx(TextInputEx, &keydata);
+                err = conInEx->ReadKeyStrokeEx(conInEx, &keydata);
                 if (EFI_ERROR(err))
                         return err;
 
@@ -116,7 +137,7 @@ EFI_STATUS console_key_read(UINT64 *key, UINT64 timeout_usec) {
                                 shift |= EFI_CONTROL_PRESSED;
                         if (keydata.KeyState.KeyShiftState & (EFI_RIGHT_ALT_PRESSED|EFI_LEFT_ALT_PRESSED))
                                 shift |= EFI_ALT_PRESSED;
-                };
+                }
 
                 /* 32 bit modifier keys + 16 bit scan code + 16 bit unicode */
                 keypress = KEYPRESS(shift, keydata.Key.ScanCode, keydata.Key.UnicodeChar);
@@ -126,14 +147,18 @@ EFI_STATUS console_key_read(UINT64 *key, UINT64 timeout_usec) {
                 }
 
                 return EFI_NOT_READY;
-        }
+        } else if (BS->CheckEvent(ST->ConIn->WaitForKey)) {
+                EFI_INPUT_KEY k;
 
-        err  = ST->ConIn->ReadKeyStroke(ST->ConIn, &k);
-        if (EFI_ERROR(err))
-                return err;
+                err = ST->ConIn->ReadKeyStroke(ST->ConIn, &k);
+                if (EFI_ERROR(err))
+                        return err;
+
+                *key = KEYPRESS(0, k.ScanCode, k.UnicodeChar);
+                return EFI_SUCCESS;
+        }
 
-        *key = KEYPRESS(0, k.ScanCode, k.UnicodeChar);
-        return EFI_SUCCESS;
+        return EFI_NOT_READY;
 }
 
 static EFI_STATUS change_mode(INT64 mode) {