All of lore.kernel.org
 help / color / mirror / Atom feed
From: ronnie sahlberg <ronniesahlberg@gmail.com>
To: Namjae Jeon <linkinjeon@kernel.org>
Cc: linux-cifs <linux-cifs@vger.kernel.org>,
	"Ralph Böhme" <slow@samba.org>,
	"Steve French" <smfrench@gmail.com>,
	"Ronnie Sahlberg" <lsahlber@redhat.com>
Subject: Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request
Date: Wed, 22 Sep 2021 10:39:32 +1000	[thread overview]
Message-ID: <CAN05THSRTAC4Na+_cMwTFEJ3WrmKQMUjq_0Tmn44RE2KHhVA3A@mail.gmail.com> (raw)
In-Reply-To: <20210921225109.6388-3-linkinjeon@kernel.org>

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
>

  reply	other threads:[~2021-09-22  0:39 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 22:51 [PATCH v2 1/3] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
2021-09-21 22:51 ` [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate Namjae Jeon
2021-09-22 14:17   ` Ralph Boehme
2021-09-22 23:13     ` Namjae Jeon
2021-09-23  0:12       ` ronnie sahlberg
2021-09-23  0:25         ` Namjae Jeon
2021-09-21 22:51 ` [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request Namjae Jeon
2021-09-22  0:39   ` ronnie sahlberg [this message]
2021-09-22  4:35     ` Namjae Jeon
2021-09-22  4:56       ` ronnie sahlberg
2021-09-22  5:35         ` Namjae Jeon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAN05THSRTAC4Na+_cMwTFEJ3WrmKQMUjq_0Tmn44RE2KHhVA3A@mail.gmail.com \
    --to=ronniesahlberg@gmail.com \
    --cc=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=lsahlber@redhat.com \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    --subject='Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.