MdeModulePkg/TerminalDxe: Fix handling of shift state in notifications

[euspectre]

Description

This patch fixes #12281.

Currently, TerminalConInRegisterKeyNotify() allows registering a notification with nonzero KeyShiftState or KeyToggleState. However, IsKeyRegistered() ignores these when checking if the keys match.

As a result, some firmware component may successfully register a notification for, say, RCtrl+n, but the notification function will be called each time the user presses 'n', which is wrong.

Most kinds of shift states and toggle states are not transferred via a terminal line, so the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger with TerminalDxe.

TerminalConInRegisterKeyNotify() could reject such notifications but it is unclear if it could break the existing UEFI components.

Instead, this patch adds a debug message when someone tries to register a notification with nonzero states, it also updates IsKeyRegistered() to take KeyShiftState and KeyToggleState into account.

This way, IsKeyRegistered() will treat the notifications for 'n' and 'RCtrl+n' as different ones. So, the callback function for 'RCtrl+n' will not be called when the user presses 'n'.

As it is not prohibited to set only EFI_SHIFT_STATE_VALID flag in the shift state (leaving the remaining bits zeroed), IsKeyRegistered() ignores EFI_SHIFT_STATE_VALID.

• Breaking change?
  • Breaking change - Does this PR cause a break in build or boot behavior?
  • Examples: Does it add a new library class or move a module to a different repo.
• Impacts security?
  • Security - Does this PR have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.
• Includes tests?
  • Tests - Does this PR include any explicit test code?
  • Examples: Unit tests or integration tests.

How This Was Tested

I tested the fix using the steps to reproduce the issue described in #12281.

Without the fix, pressing 'n' in the EFI internal shell triggered the notification for 'RCtrl+n', pressing F3 triggered the notification for 'LShift+F3'.

With the fix, neither of the notifications were triggered.

Integration Instructions

N/A

[lgao4]

@lgao4 , please review it.

[mikebeaton]

Slightly confusing, you're saying that "the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger", and then adding code to make sure that they trigger properly.

[euspectre]

Slightly confusing, you're saying that "the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger", and then adding code to make sure that they trigger properly.

No, they will not trigger with serial.

After this patch, UEFI will still allow registering such functions (but will complain about that in debug mode). However, it will now correctly compare the key event that actually happened and the ones the callback functions have been registered for. It will see the difference in the shift state (0 for the actual event and non-zero for a registered callback) and will not call such notifier functions.

This check added to IsKeyRegistered makes sure the callback for Ctrl+F3 will not trigger when F3 is pressed:
da963bd#diff-6bdbf864505f8b86109e1ed5546f42ee8617ee62189fc7785045d072e36d5132R186

[mikebeaton]

Slightly confusing, you're saying that "the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger", and then adding code to make sure that they trigger properly.

No, they will not trigger with serial.

After this patch, UEFI will still allow registering such functions (but will complain about that in debug mode). However, it will now correctly compare the key event that actually happened and the ones the callback functions have been registered for. It will see the difference in the shift state (0 for the actual event and non-zero for a registered callback) and will not call such notifier functions.

This check added to IsKeyRegistered makes sure the callback for Ctrl+F3 will not trigger when F3 is pressed: da963bd#diff-6bdbf864505f8b86109e1ed5546f42ee8617ee62189fc7785045d072e36d5132R186

No, I meant that your wording is confusing. It's a direct quote from your commit message that "the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger". At least one reading of "should not trigger" is "ought not to trigger". Which presumably you don't mean, since you're fixing to make sure they trigger properly. I am only saying that it would help to change the wording to explain what happens and what should happen, on serial and not on serial, more clearly.

Apart from the (to me, currently) confusing way it is explained, that actual changes look correct to me.

[euspectre]

No, I meant that your wording is confusing. It's a direct quote from your commit message that "the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger". At least one reading of "should not trigger" is "ought not to trigger". Which presumably you don't mean, since you're fixing to make sure they trigger properly.

Ah, I see. You are probably right.

Any ideas how to reword that to avoid confusion?

[mikebeaton]

No, I meant that your wording is confusing. It's a direct quote from your commit message that "the notification functions for the keys with nonzero KeyShiftState or KeyToggleState should not trigger". At least one reading of "should not trigger" is "ought not to trigger". Which presumably you don't mean, since you're fixing to make sure they trigger properly.

Ah, I see. You are probably right.

Any ideas how to reword that to avoid confusion?

How about just update this para of the commit message:

Shift state and toggle state are not transferred via a terminal line, so
the notification functions for the keys with nonzero KeyShiftState or
KeyToggleState should not trigger with TerminalDxe.

to

Shift state and toggle state are not transferred via a serial line, so
the notification functions for the keys with nonzero KeyShiftState or
KeyToggleState will never trigger in this case.

[euspectre]

How about something like this:

"Shift state and toggle state are not transferred via a terminal line, so it is expected that TerminalDxe never triggers the notification functions for the keys with nonzero KeyShiftState or KeyToggleState."

?

[euspectre]

Shift state and toggle state are not transferred via a serial line, so
the notification functions for the keys with nonzero KeyShiftState or
KeyToggleState will never trigger in this case.

Yes, I'll use your variant.

Thanks!

[mikebeaton]

@euspectre - Sorry, I gave a bit of a hail of comments, but if you get a moment to move the comment I suggested, I'm happy to recommend approve.

[mikebeaton]

Please could you add Fixes: #12281 in exactly that format as an additional line in the commit message. It will correctly link the issue and the PR.

[euspectre]

Fixes: #12281

Sure. Done.