[PATCH v2 1/1] efi_loader: segfault in efi_clear_os_indications()

[Ilias Apalodimas]

On Wed, 24 Nov 2021 at 09:54, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> If we call efi_clear_os_indications() before initializing the memory store
> for UEFI variables a NULL pointer dereference occurs.
>
> The error was observed on the sandbox with:
>
>     usb start
>     host bind 0 sandbox.img
>     load host 0:1 $kernel_addr_r helloworld.efi
>     bootefi $kernel_addr_r
>
> Here efi_resister_disk() failed due to an error in the BTRFS implementation.
>
> Move the logic to clear EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> to the rest of the capsule code.
>
> If CONFIG_EFI_IGNORE_OSINDICATIONS=y, we should still clear the flag.
> If OsIndications does not exist, we should not create it as it is owned by
> the operating system.
>
> Fixes: 149108a3eb59 ("efi_loader: clear OsIndications")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
> v2:
>          move the logic to the rest of the capsule code
> ---
>  lib/efi_loader/efi_capsule.c | 45 ++++++++++++++++++++++++------------
>  lib/efi_loader/efi_setup.c   | 36 +----------------------------
>  2 files changed, 31 insertions(+), 50 deletions(-)
>
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 502bcfca6e..8301eed631 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -1037,30 +1037,45 @@ efi_status_t __weak efi_load_capsule_drivers(void)
>  }
>
>  /**
> - * check_run_capsules - Check whether capsule update should run
> + * check_run_capsules() - check whether capsule update should run
>   *
>   * The spec says OsIndications must be set in order to run the capsule update
>   * on-disk.  Since U-Boot doesn't support runtime SetVariable, allow capsules to
>   * run explicitly if CONFIG_EFI_IGNORE_OSINDICATIONS is selected
> + *
> + * Return:     EFI_SUCCESS if update to run, EFI_NOT_FOUND otherwise
>   */
> -static bool check_run_capsules(void)
> +static efi_status_t check_run_capsules(void)
>  {
>         u64 os_indications;
>         efi_uintn_t size;
> -       efi_status_t ret;
> -
> -       if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS))
> -               return true;
> +       efi_status_t r;
>
>         size = sizeof(os_indications);
> -       ret = efi_get_variable_int(L"OsIndications", &efi_global_variable_guid,
> -                                  NULL, &size, &os_indications, NULL);
> -       if (ret == EFI_SUCCESS &&
> -           (os_indications
> -             & EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED))
> -               return true;
> -
> -       return false;
> +       r = efi_get_variable_int(L"OsIndications", &efi_global_variable_guid,
> +                                NULL, &size, &os_indications, NULL);
> +       if (r != EFI_SUCCESS || size != sizeof(os_indications))
> +               return EFI_NOT_FOUND;
> +
> +       if (os_indications &
> +           EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED) {
> +               os_indications &=
> +                       ~EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED;
> +               r = efi_set_variable_int(L"OsIndications",
> +                                        &efi_global_variable_guid,
> +                                        EFI_VARIABLE_NON_VOLATILE |
> +                                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +                                        EFI_VARIABLE_RUNTIME_ACCESS,
> +                                        sizeof(os_indications),
> +                                        &os_indications, false);
> +               if (r != EFI_SUCCESS)
> +                       log_err("Setting %ls failed\n", L"OsIndications");
> +               return EFI_SUCCESS;
> +       } else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) {
> +               return EFI_SUCCESS;
> +       } else  {
> +               return EFI_NOT_FOUND;
> +       }
>  }
>
>  /**
> @@ -1078,7 +1093,7 @@ efi_status_t efi_launch_capsules(void)
>         unsigned int nfiles, index, i;
>         efi_status_t ret;
>
> -       if (!check_run_capsules())
> +       if (check_run_capsules() != EFI_SUCCESS)
>                 return EFI_SUCCESS;
>
>         index = get_last_capsule();
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index a2338d74af..1aba71cd96 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -175,36 +175,6 @@ static efi_status_t efi_init_os_indications(void)
>  }
>
>
> -/**
> - * efi_clear_os_indications() - clear OsIndications
> - *
> - * Clear EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> - */
> -static efi_status_t efi_clear_os_indications(void)
> -{
> -       efi_uintn_t size;
> -       u64 os_indications;
> -       efi_status_t ret;
> -
> -       size = sizeof(os_indications);
> -       ret = efi_get_variable_int(L"OsIndications", &efi_global_variable_guid,
> -                                  NULL, &size, &os_indications, NULL);
> -       if (ret != EFI_SUCCESS)
> -               os_indications = 0;
> -       else
> -               os_indications &=
> -                       ~EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED;
> -       ret = efi_set_variable_int(L"OsIndications", &efi_global_variable_guid,
> -                                  EFI_VARIABLE_NON_VOLATILE |
> -                                  EFI_VARIABLE_BOOTSERVICE_ACCESS |
> -                                  EFI_VARIABLE_RUNTIME_ACCESS,
> -                                  sizeof(os_indications), &os_indications,
> -                                  false);
> -       if (ret != EFI_SUCCESS)
> -               log_err("Setting %ls failed\n", L"OsIndications");
> -       return ret;
> -}
> -
>  /**
>   * efi_init_obj_list() - Initialize and populate EFI object list
>   *
> @@ -212,7 +182,7 @@ static efi_status_t efi_clear_os_indications(void)
>   */
>  efi_status_t efi_init_obj_list(void)
>  {
> -       efi_status_t r, ret = EFI_SUCCESS;
> +       efi_status_t ret = EFI_SUCCESS;
>
>         /* Initialize once only */
>         if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
> @@ -331,11 +301,7 @@ efi_status_t efi_init_obj_list(void)
>         if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) &&
>             !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY))
>                 ret = efi_launch_capsules();
> -
>  out:
> -       r = efi_clear_os_indications();
> -       if (ret == EFI_SUCCESS)
> -               ret = r;
>         efi_obj_list_initialized = ret;
>         return ret;
>  }
> --
> 2.32.0
>

Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>