[U-Boot] [RFC] efi_loader: workaround for EDK2's shell.efi
The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent") enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
The patch provides a minimum mitigation of parameter check.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- lib/efi_loader/efi_boottime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2281703f261..e7a19c35415 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl, return EFI_INVALID_PARAMETER; }
- if (is_valid_tpl(notify_tpl) != EFI_SUCCESS) + /* notify_tpl == 0: workaround for EDK2's Shell.efi */ + if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS)) return EFI_INVALID_PARAMETER;
evt = calloc(1, sizeof(struct efi_event));
On 09.08.18 07:15, AKASHI Takahiro wrote:
The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent") enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
... of the EDK2 shell ...
and it's not just about short term - we always want to be compatible :).
So what's the reason this does not trigger in edk2? Are they considering TPL 0 a valid TPL always or did they just forget the check in create event? If they always consider TPL 0 valid, we better change is_valid_tpl to ensure compatibility with edk2's behavior.
The patch provides a minimum mitigation of parameter check.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_boottime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2281703f261..e7a19c35415 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl, return EFI_INVALID_PARAMETER; }
- if (is_valid_tpl(notify_tpl) != EFI_SUCCESS)
- /* notify_tpl == 0: workaround for EDK2's Shell.efi */
That comment is too undescriptive. Better write something like "EDK2 accepts TPL 0 in CreateEvent, so to ensure compatibility we should do the same. EDK2 Shell.efi depends on this."
Alex
if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS)) return EFI_INVALID_PARAMETER;
evt = calloc(1, sizeof(struct efi_event));
On Thu, Aug 09, 2018 at 07:55:06AM +0100, Alexander Graf wrote:
On 09.08.18 07:15, AKASHI Takahiro wrote:
The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent") enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
... of the EDK2 shell ...
and it's not just about short term - we always want to be compatible :).
Okay.
So what's the reason this does not trigger in edk2? Are they considering TPL 0 a valid TPL always or did they just forget the check in create event? If they always consider TPL 0 valid, we better change is_valid_tpl to ensure compatibility with edk2's behavior.
I'm not confident about what Shell's intent is. Created here is an event to be used to raise a signal for "notification of Ctrl-C keystrokes," and hence Shell expects such key data to always be sent to a task whatever its TPL is?
The patch provides a minimum mitigation of parameter check.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_boottime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2281703f261..e7a19c35415 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl, return EFI_INVALID_PARAMETER; }
- if (is_valid_tpl(notify_tpl) != EFI_SUCCESS)
- /* notify_tpl == 0: workaround for EDK2's Shell.efi */
That comment is too undescriptive. Better write something like "EDK2 accepts TPL 0 in CreateEvent, so to ensure compatibility we should do the same. EDK2 Shell.efi depends on this."
Nice!
Thanks, -Takahiro AKASHI
Alex
if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS)) return EFI_INVALID_PARAMETER;
evt = calloc(1, sizeof(struct efi_event));
Am 09.08.2018 um 09:30 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Aug 09, 2018 at 07:55:06AM +0100, Alexander Graf wrote:
On 09.08.18 07:15, AKASHI Takahiro wrote: The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent") enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
... of the EDK2 shell ...
and it's not just about short term - we always want to be compatible :).
Okay.
So what's the reason this does not trigger in edk2? Are they considering TPL 0 a valid TPL always or did they just forget the check in create event? If they always consider TPL 0 valid, we better change is_valid_tpl to ensure compatibility with edk2's behavior.
I'm not confident about what Shell's intent is. Created here is an event to be used to raise a signal for "notification of Ctrl-C keystrokes," and hence Shell expects such key data to always be sent to a task whatever its TPL is?
Leif, can you please help out here?
Thanks!
Alex
On 08/09/2018 08:15 AM, AKASHI Takahiro wrote:
The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent")
scripts/checkpatch.pl wants 12 digits for the commit reference.
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")'
Please, check patches before submitting.
The commit 21b3edfc9644 ("efi_loader: check parameters of CreateEvent")
enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
The patch provides a minimum mitigation of parameter check.
This patch relates to test number 5.1.1.1.7 in Self Certification Test (SCT) II Case Specification June 2017.
Fixes: 21b3edfc9644 ("efi_loader: check parameters of CreateEvent")
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_boottime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2281703f261..e7a19c35415 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl, return EFI_INVALID_PARAMETER; }
- if (is_valid_tpl(notify_tpl) != EFI_SUCCESS)
- /* notify_tpl == 0: workaround for EDK2's Shell.efi */
- if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS))
Thanks for catching this.
The UEFI 2.7 spec has the following parameter description:
NotifyTpl: The task priority level of event notifications, if needed.
CreateEvent is implemented in EDK2 CoreCreateEvent() which calls CoreCreateEventEx(). The latter has the following test:
if ((Type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL)) != 0) { if (NotifyTpl != TPL_APPLICATION && NotifyTpl != TPL_CALLBACK && NotifyTpl != TPL_NOTIFY) { return EFI_INVALID_PARAMETER; } }
In my patch I missed to check parameter Type first.
Please, change your patch so that it matches what test case 5.1.1.1.7 checks (i.e. the EDK 2 logic).
Best regards
Heinrich
return EFI_INVALID_PARAMETER;evt = calloc(1, sizeof(struct efi_event));
On Thu, Aug 09, 2018 at 03:15:38PM +0900, AKASHI Takahiro wrote:
The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent") enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
I find this done in CreatePopulateInstallShellProtocol() in Application/Shell/ShellProtocol.c, is that the one you see?
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
Where we find non-spec-compliant code in EDK2, we want to fix EDK2. That doesn't mean that we don't perhaps want to work around it in U-Boot anyway. But if we do, I would prefer if we could spam the console a bit as well, to warn people of badly behaving apps.
However...
The patch provides a minimum mitigation of parameter check.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_boottime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2281703f261..e7a19c35415 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl, return EFI_INVALID_PARAMETER; }
- if (is_valid_tpl(notify_tpl) != EFI_SUCCESS)
- /* notify_tpl == 0: workaround for EDK2's Shell.efi */
- if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS))
From the UEFI spec (2.7) description of CreateEvent() boot service:
--- The EVT_NOTIFY_WAIT and EVT_NOTIFY_SIGNAL flags are exclusive. If neither flag is specified, the caller does not require any notification concerning the event and the NotifyTpl, NotifyFunction, and NotifyContext parameters are ignored. ---
So it's not a workaround for Shell specifically. However, based on that text, something like
if (type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL)) if ((is_valid_tpl(notify_tpl) != EFI_SUCCESS))
may resolve this in a more compliant way.
Of course, this may require additional changes to the remainder of the function.
/ Leif
return EFI_INVALID_PARAMETER;evt = calloc(1, sizeof(struct efi_event));
2.18.0
Leif, Heinrich,
Thank you for your comments. I should have been more careful in reading UEFI specification :)
On Thu, Aug 09, 2018 at 02:08:32PM +0100, Leif Lindholm wrote:
On Thu, Aug 09, 2018 at 03:15:38PM +0900, AKASHI Takahiro wrote:
The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent") enforces a strict parameter check at CreateEvent(). Unfortunately, however, EDK2's Shell.efi calls this function with notify_tpl == 0.
I find this done in CreatePopulateInstallShellProtocol() in Application/Shell/ShellProtocol.c, is that the one you see?
Right.
The patch above does right thing and we'd better fix the issue on EDK2 side, and yet we might want a workaround allowing for running un-modified version of EDK2 in short-term solution.
Where we find non-spec-compliant code in EDK2, we want to fix EDK2. That doesn't mean that we don't perhaps want to work around it in U-Boot anyway. But if we do, I would prefer if we could spam the console a bit as well, to warn people of badly behaving apps.
However...
The patch provides a minimum mitigation of parameter check.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_boottime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2281703f261..e7a19c35415 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl, return EFI_INVALID_PARAMETER; }
- if (is_valid_tpl(notify_tpl) != EFI_SUCCESS)
- /* notify_tpl == 0: workaround for EDK2's Shell.efi */
- if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS))
From the UEFI spec (2.7) description of CreateEvent() boot service:
The EVT_NOTIFY_WAIT and EVT_NOTIFY_SIGNAL flags are exclusive. If neither flag is specified, the caller does not require any notification concerning the event and the NotifyTpl, NotifyFunction, and NotifyContext parameters are ignored.
So it's not a workaround for Shell specifically. However, based on that text, something like
if (type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL)) if ((is_valid_tpl(notify_tpl) != EFI_SUCCESS))may resolve this in a more compliant way.
OK. I will respin my patch, also addressing Heinrich's comments.
-Takahiro AKASHI
Of course, this may require additional changes to the remainder of the function.
/ Leif
return EFI_INVALID_PARAMETER;evt = calloc(1, sizeof(struct efi_event));
2.18.0
participants (4)
-
AKASHI Takahiro -
Alexander Graf -
Heinrich Schuchardt -
Leif Lindholm