Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAKv+Gu_3ZRRcoAcLTVVQe26q5x9KALmztaNQF=e=KqWaAwxtpA@mail.gmail.com>
 <20200304154936.24206-1-vdronov@redhat.com>
In-Reply-To: <20200304154936.24206-1-vdronov@redhat.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 4 Mar 2020 16:57:16 +0100
Message-ID: <CAKv+Gu_WFL24dGPakcPgGW3MayYx1qND9HxL87vods7h4LyZJw@mail.gmail.com>
Subject: Re: [PATCH v2] efi: fix a race and a buffer overflow while reading
 efivars via sysfs
To:     Vladis Dronov <vdronov@redhat.com>
Cc:     linux-efi <linux-efi@vger.kernel.org>, joeyli <jlee@suse.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 4 Mar 2020 at 16:50, Vladis Dronov <vdronov@redhat.com> wrote:
>
> There is a race and a buffer overflow corrupting a kernel memory while
> reading an efi variable with a size more than 1024 bytes via the older
> sysfs method. This happens because accessing struct efi_variable in
> efivar_{attr,size,data}_read() and friends is not protected from
> a concurrent access leading to a kernel memory corruption and, at best,
> to a crash. The race scenario is the following:
>
> CPU0:                                CPU1:
> efivar_attr_read()
>   var->DataSize = 1024;
>   efivar_entry_get(... &var->DataSize)
>     down_interruptible(&efivars_lock)
>                                      efivar_attr_read() // same efi var
>                                        var->DataSize = 1024;
>                                        efivar_entry_get(... &var->DataSize)
>                                          down_interruptible(&efivars_lock)
>     virt_efi_get_variable()
>     // returns EFI_BUFFER_TOO_SMALL but
>     // var->DataSize is set to a real
>     // var size more than 1024 bytes
>     up(&efivars_lock)
>                                          virt_efi_get_variable()
>                                          // called with var->DataSize set
>                                          // to a real var size, returns
>                                          // successfully and overwrites
>                                          // a 1024-bytes kernel buffer
>                                          up(&efivars_lock)
>
> This can be reproduced by concurrent reading of an efi variable which size
> is more than 1024 bytes:
>
> ts# for cpu in $(seq 0 $(nproc --ignore=1)); do ( taskset -c $cpu \
> cat /sys/firmware/efi/vars/KEKDefault*/size & ) ; done
>
> Fix this by using a local variable for a var's data buffer size so it
> does not get overwritten. Also add a sanity check to efivar_store_raw().
>
> Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
>  drivers/firmware/efi/efi-pstore.c |  2 +-
>  drivers/firmware/efi/efivars.c    | 32 ++++++++++++++++++++++---------
>  drivers/firmware/efi/vars.c       |  2 +-
>  3 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index 9ea13e8d12ec..e4767a7ce973 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -161,7 +161,7 @@ static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
>   *
>   * @record: pstore record to pass to callback
>   *
> - * You MUST call efivar_enter_iter_begin() before this function, and
> + * You MUST call efivar_entry_iter_begin() before this function, and
>   * efivar_entry_iter_end() afterwards.
>   *
>   */

This hunk can be dropped now, I guess

> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..16a617f9c5cf 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -83,13 +83,16 @@ static ssize_t
>  efivar_attr_read(struct efivar_entry *entry, char *buf)
>  {
>         struct efi_variable *var = &entry->var;
> +       unsigned long size = sizeof(var->Data);
>         char *str = buf;
> +       int ret;
>
>         if (!entry || !buf)
>                 return -EINVAL;
>
> -       var->DataSize = 1024;
> -       if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> +       ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> +       var->DataSize = size;

For my understanding, could you explain why we do the assignment here?
Does var->DataSize matter in this case? Can it deviate from 1024?

> +       if (ret)
>                 return -EIO;
>
>         if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
> @@ -116,13 +119,16 @@ static ssize_t
>  efivar_size_read(struct efivar_entry *entry, char *buf)
>  {
>         struct efi_variable *var = &entry->var;
> +       unsigned long size = sizeof(var->Data);
>         char *str = buf;
> +       int ret;
>
>         if (!entry || !buf)
>                 return -EINVAL;
>
> -       var->DataSize = 1024;
> -       if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> +       ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> +       var->DataSize = size;
> +       if (ret)
>                 return -EIO;
>
>         str += sprintf(str, "0x%lx\n", var->DataSize);
> @@ -133,12 +139,15 @@ static ssize_t
>  efivar_data_read(struct efivar_entry *entry, char *buf)
>  {
>         struct efi_variable *var = &entry->var;
> +       unsigned long size = sizeof(var->Data);
> +       int ret;
>
>         if (!entry || !buf)
>                 return -EINVAL;
>
> -       var->DataSize = 1024;
> -       if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> +       ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> +       var->DataSize = size;
> +       if (ret)
>                 return -EIO;
>
>         memcpy(buf, var->Data, var->DataSize);
> @@ -199,6 +208,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
>         u8 *data;
>         int err;
>
> +       if (!entry || !buf)
> +               return -EINVAL;
> +

So what are we sanity checking here? When might this occur? Does it
need to be in the same patch?

>         if (in_compat_syscall()) {
>                 struct compat_efi_variable *compat;
>
> @@ -250,14 +262,16 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
>  {
>         struct efi_variable *var = &entry->var;
>         struct compat_efi_variable *compat;
> +       unsigned long datasize = sizeof(var->Data);
>         size_t size;
> +       int ret;
>
>         if (!entry || !buf)
>                 return 0;
>
> -       var->DataSize = 1024;
> -       if (efivar_entry_get(entry, &entry->var.Attributes,
> -                            &entry->var.DataSize, entry->var.Data))
> +       ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
> +       var->DataSize = size;
> +       if (ret)
>                 return -EIO;
>
>         if (in_compat_syscall()) {
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 436d1776bc7b..5f2a4d162795 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -1071,7 +1071,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
>   * entry on the list. It is safe for @func to remove entries in the
>   * list via efivar_entry_delete().
>   *
> - * You MUST call efivar_enter_iter_begin() before this function, and
> + * You MUST call efivar_entry_iter_begin() before this function, and
>   * efivar_entry_iter_end() afterwards.
>   *
>   * It is possible to begin iteration from an arbitrary entry within

We can drop this.

> --
> 2.20.1
>