On Sun, 26 Jun 2022 at 12:32, Hyunwoo Kim <imv4bel@xxxxxxxxx> wrote:
>
> If the user calls close() during a copy operation in copy_from_user() of efi_capsule_write(),
> a race condition may occur in which the user's buffer is copied to the freed page.
>
> This is because .flush of file_operations is called unconditionally
> regardless of ->f_count, unlike .release.
>
> This driver is writable only with root privileges, so it is not a security vulnerability.
> However, it is recommended to add mutexes to efi_capsule_write() and efi_capsule_flush()
> as root can accidentally break the page while in use.
>
Apologies for the late reply.
Could you please elaborate? I.e., describe in more detail how the race
condition may occur?
Thanks,
> Signed-off-by: Hyunwoo Kim <imv4bel@xxxxxxxxx>
> ---
> drivers/firmware/efi/capsule-loader.c | 12 ++++++++++++
> include/linux/efi.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 4dde8edd53b6..e50ede51ef38 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -177,6 +177,8 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
> if (count == 0)
> return 0;
>
> + mutex_lock(&cap_info->write_lock);
> +
> /* Return error while NO_FURTHER_WRITE_ACTION is flagged */
> if (cap_info->index < 0)
> return -EIO;
> @@ -233,12 +235,16 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
> goto failed;
> }
>
> + mutex_unlock(&cap_info->write_lock);
> +
> return write_byte;
>
> fail_unmap:
> kunmap(page);
> failed:
> efi_free_all_buff_pages(cap_info);
> + mutex_unlock(&cap_info->write_lock);
> +
> return ret;
> }
>
> @@ -256,12 +262,16 @@ static int efi_capsule_flush(struct file *file, fl_owner_t id)
> int ret = 0;
> struct capsule_info *cap_info = file->private_data;
>
> + mutex_lock(&cap_info->write_lock);
> +
> if (cap_info->index > 0) {
> pr_err("capsule upload not complete\n");
> efi_free_all_buff_pages(cap_info);
> ret = -ECANCELED;
> }
>
> + mutex_unlock(&cap_info->write_lock);
> +
> return ret;
> }
>
> @@ -315,6 +325,8 @@ static int efi_capsule_open(struct inode *inode, struct file *file)
> return -ENOMEM;
> }
>
> + mutex_init(&cap_info->write_lock);
> +
> file->private_data = cap_info;
>
> return 0;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 7d9b0bb47eb3..e274c4e8d7c6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -204,6 +204,7 @@ struct efi_image_auth {
> struct capsule_info {
> efi_capsule_header_t header;
> efi_capsule_header_t *capsule;
> + struct mutex write_mutex;
> int reset_type;
> long index;
> size_t count;
> --
> 2.25.1
>
> Dear all,
>
> I submitted this patch 2 weeks ago, this is my 3rd submission of this patch.
>
> Can I get feedback on this patch?
>
> Regards,
> Hyunwoo Kim.
