linux-kernel - Re: [PATCH] CIFS: Fix bug which the return value by asynchronous read is error

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:   Wed, 18 Mar 2020 14:47:32 +1000
From:   ronnie sahlberg <ronniesahlberg@...il.com>
To:     Yilu Lin <linyilu@...wei.com>
Cc:     linux-cifs <linux-cifs@...r.kernel.org>,
        Steve French <sfrench@...ba.org>,
        "samba-technical@...ts.samba.org" <samba-technical@...ts.samba.org>,
        LKML <linux-kernel@...r.kernel.org>, alex.chen@...wei.com
Subject: Re: [PATCH] CIFS: Fix bug which the return value by asynchronous read
 is error

Hi Yilu,

I think your reasoning makes sense.
Do you have a small reproducer for this? A small C program that triggers this?

I am asking because if you do we would like to add it to our buildbot
to make  sure we don't get regressions.


regards
ronnie sahlberg

On Wed, Mar 18, 2020 at 1:59 PM Yilu Lin <linyilu@...wei.com> wrote:
>
> This patch is used to fix the bug in collect_uncached_read_data()
> that rc is automatically converted from a signed number to an
> unsigned number when the CIFS asynchronous read fails.
> It will cause ctx->rc is error.
>
> Example:
> Share a directory and create a file on the Windows OS.
> Mount the directory to the Linux OS using CIFS.
> On the CIFS client of the Linux OS, invoke the pread interface to
> deliver the read request.
>
> The size of the read length plus offset of the read request is greater
> than the maximum file size.
>
> In this case, the CIFS server on the Windows OS returns a failure
> message (for example, the return value of
> smb2.nt_status is STATUS_INVALID_PARAMETER).
>
> After receiving the response message, the CIFS client parses
> smb2.nt_status to STATUS_INVALID_PARAMETER
> and converts it to the Linux error code (rdata->result=-22).
>
> Then the CIFS client invokes the collect_uncached_read_data function to
> assign the value of rdata->result to rc, that is, rc=rdata->result=-22.
>
> The type of the ctx->total_len variable is unsigned integer,
> the type of the rc variable is integer, and the type of
> the ctx->rc variable is ssize_t.
>
> Therefore, during the ternary operation, the value of rc is
> automatically converted to an unsigned number. The final result is
> ctx->rc=4294967274. However, the expected result is ctx->rc=-22.
>
> Signed-off-by: Yilu Lin <linyilu@...wei.com>
> ---
>  fs/cifs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 022029a5d..ff4ac244c 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -3323,7 +3323,7 @@ again:
>         if (rc == -ENODATA)
>                 rc = 0;
>
> -       ctx->rc = (rc == 0) ? ctx->total_len : rc;
> +       ctx->rc = (rc == 0) ? (ssize_t)ctx->total_len : rc;
>
>         mutex_unlock(&ctx->aio_mutex);
>
> --
> 2.19.1
>
>