Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request

[ronnie sahlberg <ronniesahlberg@gmail.com>]

On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> Ronnie reported invalid request buffer access in chained command when
> inserting garbage value to NextCommand of compound request.
> This patch add validation check to avoid this issue.
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  v2:
>   - fix integer overflow from work->next_smb2_rcv_hdr_off.
>
>  fs/ksmbd/smb2pdu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 1fe37ad4e5bc..cae796ea1148 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work *work)
>
>         hdr = ksmbd_req_buf_next(work);
>         if (le32_to_cpu(hdr->NextCommand) > 0) {
> +               if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) >
> +                   get_rfc1002_len(work->request_buf)) {
> +                       pr_err("next command(%u) offset exceeds smb msg size\n",
> +                              hdr->NextCommand);
> +                       return false;
> +               }
> +
>                 ksmbd_debug(SMB, "got SMB2 chained command\n");
>                 init_chained_smb2_rsp(work);
>                 return true;

Very good, reviewed by me.
The conditional though, since you know there will be at least a full
smb2 header there you could already check that change it to
> +               if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) >
> +                   get_rfc1002_len(work->request_buf) +  64) {


Which leads to another question.  Where do you check that the buffer
contains enough data to hold the smb2 header and the full fixed part
of the request?
There is a check that you have enough space for the smb2 header in
ksmbd_conn_handler_loop()
that there is enough space for the smb2 header
(ksmbd_pdu_size_has_room()) but that function assumes that the smb2
header always start at the head of the buffer.
So if you have a compound chain, this functrion only checks the first pdu.


I know that the buffer handling is copied from the cifs client.  It
used to also do these "just pass a buffer around and the first 4 bytes
is the size" (and still does for smb1)  and there was a lot of
terrible +4 or -4 to all sort of casts and conditionals.
I changed that in cifs.ko to remove the 4 byte length completely from
the buffer.
I also changed it as part of the compounding to pass an array of
requests (each containing an iovector) to the functions instead of
just one large byte array.
That made things a lot easier to manage since you could then assume
that the SMB2 header would always start at offset 0 in the
corresponding iovector, even for compounded commands since they all
had their own private vector.
And since an iovector contains both a pointer and a length there is no
need anymore to read the first 4 bytes/validate them/and covnert into
a length all the time.

I think that would help, but it would be a MAJOR amount of work, so
maybe that should wait until later.
That approach is very nice since it completely avoids keeping track of
offset-to-where-this-pdu-starts which makes all checks and
conditionals so much more complex.


regards
ronnie sahlberg


> --
> 2.25.1
>