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 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.

> >
> >
> > 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
> >>
> >