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>
 <20200305084041.24053-1-vdronov@redhat.com> <20200305084041.24053-2-vdronov@redhat.com>
In-Reply-To: <20200305084041.24053-2-vdronov@redhat.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Thu, 5 Mar 2020 09:45:35 +0100
Message-ID: <CAKv+Gu_n8MhgRFw-BUFgN1UUfTh1R6wsCNxKRA9QrQK74z6g7g@mail.gmail.com>
Subject: Re: [PATCH v3 1/3] 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 Thu, 5 Mar 2020 at 09:41, 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.
>
> Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> Link: https://lore.kernel.org/linux-efi/20200303085528.27658-1-vdronov@redhat.com/T/#u

For the future, please don't add these links. This one points to the
old version of the patch, not to this one. It will be added by the
tooling once the patch gets picked up.

> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
>  drivers/firmware/efi/efivars.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..69f13bc4b931 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;
> +       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);
> @@ -250,14 +259,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 = datasize;
> +       if (ret)
>                 return -EIO;
>
>         if (in_compat_syscall()) {
> --
> 2.20.1
>