Re: [PATCH 8/9] efi: Only register shim_lock verifier if shim_lock protocol is found and SB enabled

[Michael Chang]

On Thu, Dec 03, 2020 at 04:01:49PM +0100, Javier Martinez Canillas wrote:
> The shim_lock module registers a verifier to call shim's verify, but the
> handler is registered even when the shim_lock protocol was not installed.
> 
> This doesn't cause a NULL pointer dereference in shim_lock_write() because
> the shim_lock_init() function just returns GRUB_ERR_NONE if sl isn't set.
> 
> But in that case there's no point to even register the shim_lock verifier
> since won't do anything. Additionally, it is only useful when Secure Boot
> is enabled.
> 
> Finally, don't assume that the shim_lock protocol will always be present
> when the shim_lock_write() function is called, and check for it on every
> call to this function.
> 
> Reported-by: Michael Chang <mchang@suse.com>

To complete the information here, this fixed the problem I tried to
solve before, but in a more elegant way. :)

https://www.mail-archive.com/grub-devel@gnu.org/msg30738.html

Thank you to work on the patch.

Regards,
Michael

> Reported-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  grub-core/commands/efi/shim_lock.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/commands/efi/shim_lock.c 
> b/grub-core/commands/efi/shim_lock.c
> index d8f52d721c3..5259b27e8fc 100644
> --- a/grub-core/commands/efi/shim_lock.c
> +++ b/grub-core/commands/efi/shim_lock.c
> @@ -28,7 +28,6 @@
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
>  static grub_efi_guid_t shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
> -static grub_efi_shim_lock_protocol_t *sl;
>  
>  /* List of modules which cannot be loaded if UEFI secure boot mode is 
> enabled. */
>  static const char * const disabled_mods[] = {"iorw", "memrw", "wrmsr", NULL};
> @@ -43,9 +42,6 @@ shim_lock_init (grub_file_t io, enum grub_file_type type,
>  
>    *flags = GRUB_VERIFY_FLAGS_SKIP_VERIFICATION;
>  
> -  if (!sl)
> -    return GRUB_ERR_NONE;
> -
>    switch (type & GRUB_FILE_TYPE_MASK)
>      {
>      case GRUB_FILE_TYPE_GRUB_MODULE:
> @@ -100,6 +96,11 @@ shim_lock_init (grub_file_t io, enum grub_file_type type,
>  static grub_err_t
>  shim_lock_write (void *context __attribute__ ((unused)), void *buf, 
> grub_size_t size)
>  {
> +  grub_efi_shim_lock_protocol_t *sl = grub_efi_locate_protocol 
> (&shim_lock_guid, 0);
> +
> +  if (sl == NULL)
> +    return grub_error (GRUB_ERR_ACCESS_DENIED, N_("shim_lock protocol not 
> found"));
> +
>    if (sl->verify (buf, size) != GRUB_EFI_SUCCESS)
>      return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad shim signature"));
>  
> @@ -115,12 +116,13 @@ struct grub_file_verifier shim_lock =
>  
>  GRUB_MOD_INIT(shim_lock)
>  {
> -  sl = grub_efi_locate_protocol (&shim_lock_guid, 0);
> -  grub_verifier_register (&shim_lock);
> +  grub_efi_shim_lock_protocol_t *sl = grub_efi_locate_protocol 
> (&shim_lock_guid, 0);
>  
> -  if (!sl)
> +  if (sl == NULL || grub_efi_get_secureboot () != 
> GRUB_EFI_SECUREBOOT_MODE_ENABLED)
>      return;
>  
> +  grub_verifier_register (&shim_lock);
> +
>    grub_dl_set_persistent (mod);
>  }
>  
> -- 
> 2.28.0
>