All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namjae Jeon <linkinjeon@kernel.org>
To: ronnie sahlberg <ronniesahlberg@gmail.com>
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 14:35:12 +0900	[thread overview]
Message-ID: <CAKYAXd-_DXbu6q_osn5_qq9yie8ZCjRnyCMjF=bFdgraD7Pcfw@mail.gmail.com> (raw)
In-Reply-To: <CAN05THST2Avwo4PebF4YmH65SrO-T3nS=jrs5cQMwT-vCW17-Q@mail.gmail.com>

2021-09-22 13:56 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
> On Wed, Sep 22, 2021 at 2:35 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> 2021-09-22 9:39 GMT+09:00, 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.
>> Sorry for late response, Thanks for your review!
>> > 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) {
>> Ah, I didn't understand why we should add + 64(smb2 hdr size)...
>> As I know, NextCommand offset included smb2 header size..
>
> This is what I meant.
> +               if ((u64)work->next_smb2_rcv_hdr_off +
> le32_to_cpu(hdr->NextCommand) + 64 >
> +                   get_rfc1002_len(work->request_buf)) {
>
> It could just be an early check that what hdr->NextCommand points to
> has at least 64 bytes.
> I.e. an early test that "does the next PDU have at least a full smb2
> header?"
>
> I mean, since you already test that NextCommand is valid,  you could
> at the same time also
> test that the next pdu is at least 64 bytes.
Understood, I will update it on v3.

Thanks!
>
>> >
>> >
>> > 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?
>> ksmbd_smb2_check_message() in smb2misc.c should check it.
>>
>> > 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 think that is_chained_smb2_message() will check all pdu as well as first
>> pdu.
>> there is loop do { } while (is_chained_smb2_message(work)); in server.c
>> >
>> >
>> > 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.
>> Right. fully agreed.
>>
>> >
>> > I think that would help, but it would be a MAJOR amount of work, so
>> > maybe that should wait until later.
>> Agreed, I will do that after fixing current urgent issues first!
>>
>> > 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.
>> Thanks!
>> >
>> >
>> > regards
>> > ronnie sahlberg
>> >
>> >
>> >> --
>> >> 2.25.1
>> >>
>> >
>

      reply	other threads:[~2021-09-22  5:35 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
2021-09-22  4:35     ` Namjae Jeon
2021-09-22  4:56       ` ronnie sahlberg
2021-09-22  5:35         ` Namjae Jeon [this message]

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='CAKYAXd-_DXbu6q_osn5_qq9yie8ZCjRnyCMjF=bFdgraD7Pcfw@mail.gmail.com' \
    --to=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=lsahlber@redhat.com \
    --cc=ronniesahlberg@gmail.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.