linux-kernel - Re: [PATCH v1 2/2] fsi: sbefifo: Validate pending user write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Sep 2023 05:52:25 +0000
From:   Joel Stanley <joel@....id.au>
To:     Ninad Palsule <ninad@...ux.ibm.com>, eajames@...ux.ibm.com
Cc:     jk@...abs.org, alistair@...ple.id.au, linux-fsi@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] fsi: sbefifo: Validate pending user write

On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@...ux.ibm.com> wrote:
>
> This commit fails user write operation if previous write operation is
> still pending.
>
> As per the driver design write operation only prepares the buffer, the
> actual FSI write is performed on next read operation. so if buggy
> application sends two back to back writes or two parallel writes then
> that could cause memory leak.

The driver already has this code:


>
> Signed-off-by: Ninad Palsule <ninad@...ux.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index b771dff27f7f..824e2a921a25 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -874,6 +874,12 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>
>         mutex_lock(&user->file_lock);
>
> +       /* Previous write is still in progress */
> +       if (user->pending_cmd) {
> +               mutex_unlock(&user->file_lock);
> +               return -EALREADY;

That's an unusual return code. I guess it makes sense in this context.

It's good to fix the potential memory leak, and we should add code to
catch that case.

This will change the behaviour of the character device from "overwrite
the previous operation" to "reject operation until a read is
performed". Do you think there's existing code that depends on the old
behaviour?

> +       }
> +
>         /* Can we use the pre-allocate buffer ? If not, allocate */
>         if (len <= PAGE_SIZE)
>                 user->pending_cmd = user->cmd_page;
> --
> 2.39.2
>