2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
> On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@xxxxxxxxxx> 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@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Reported-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> 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..
>
>
> 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
>>
>
