Re: [edk2-devel] [PATCH v2 12/12] OvmfPkg: only add shell to FV in case secure boot is disabled

["Laszlo Ersek" <lersek@redhat.com>]

On 2/15/24 11:25, Gerd Hoffmann wrote:
> The EFI Shell allows to bypass secure boot, do not allow
> to include the shell in the firmware images of secure boot
> enabled builds.
>
> This prevents misconfigured downstream builds.
>
> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/2040137
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4641
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Include/Fdf/ShellDxe.fdf.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Include/Fdf/ShellDxe.fdf.inc b/OvmfPkg/Include/Fdf/ShellDxe.fdf.inc
> index 3081ac41780a..38f69747b02c 100644
> --- a/OvmfPkg/Include/Fdf/ShellDxe.fdf.inc
> +++ b/OvmfPkg/Include/Fdf/ShellDxe.fdf.inc
> @@ -2,7 +2,7 @@
>  #    SPDX-License-Identifier: BSD-2-Clause-Patent
>  ##
>
> -!if $(BUILD_SHELL) == TRUE
> +!if $(BUILD_SHELL) == TRUE && $(SECURE_BOOT_ENABLE) == FALSE
>
>  !if $(TOOL_CHAIN_TAG) != "XCODE5"
>  !if $(NETWORK_ENABLE) == TRUE

This does the job:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

An alternative could be (perhaps informing the user better):

  !if $(BUILD_SHELL) == TRUE
  !if $(SECURE_BOOT_ENABLE) == TRUE
  !error BUILD_SHELL and SECURE_BOOT_ENABLE conflict
  !endif
  ...
  !endif

If you prefer that approach, feel free to submit a v3 (you can add my
R-b for that variant immediately).

A disadvantage might be that, with such a patch in place, people having
built OVMF with "-D SECURE_BOOT_ENABLE" thus far would start getting
build failures, and they'd have to explicitly pass "-D
BUILD_SHELL=FALSE", IIUC.

(Well, some might consider such a build failure an advantage actually:
near the BUILD_SHELL define, we have a comment saying "Shell can be
useful for debugging but should not be enabled for production". Pick
your poison, I guess...)

A repost might be worth your while either way, because some of the
patches are identical to their first versions, and Jiewen's v1 Acked-by,
from [1], is missing from the unchanged (or trivially rebased) patches
in v2.

I strongly prefer capturing all feedback tags in the git history; after
all, that keeps a record of reviewer *work* (not just the mere approval
itself).

[1] https://edk2.groups.io/g/devel/message/114362
    msgid <MW4PR11MB5872132F77F29DC6758C71178C7A2@MW4PR11MB5872.namprd11.prod.outlook.com>

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115551): https://edk2.groups.io/g/devel/message/115551
Mute This Topic: https://groups.io/mt/104370218/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-