From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Fri, 10 Aug 2018 09:15:34 +0900
Subject: [U-Boot] [RFC] efi_loader: workaround for EDK2's shell.efi
In-Reply-To: <20180809130832.cgihbbrytnvk6qvr@bivouac.eciton.net>
References: <20180809061538.6624-1-takahiro.akashi@linaro.org>
 <20180809130832.cgihbbrytnvk6qvr@bivouac.eciton.net>
Message-ID: <20180810001532.GT11258@linaro.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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