2023-05-25 21:18 GMT+09:00, 張智諺 <cc85nod@xxxxxxxxx>:
> `len_of_ctxts` is signed and 4 bytes (int), and `sizeof(struct
> smb2_neg_context)` is unsigned and 8 bytes (unsigned long).
>
> When comparing values with different size and different sign, first
> compiler will add instruction cdqe to extend small one to 8 bytes,
> then casting signed one to unsigned, which is `len_of_ctxts` in this case.
> So a negative `len_of_ctxts` will be viewed
> as a large unsigned value, which is totally larger than `sizeof(struct
> smb2_neg_context)`.
Okay, Can you update this check instead of adding new check ?
>
> I use program below to verify my guess:
>
> ```
> #include <stdio.h>
>
> int main()
> {
> int a = -4;
> unsigned long b = 0x8;
>
> if (a < b)
> printf("expected\n"); // will not print
> }
> ```
>
> Namjae Jeon <linkinjeon@xxxxxxxxxx> 於 2023年5月25日 週四 下午7:49寫道：
>
>> 2023-05-25 14:31 GMT+09:00, 張智諺 <cc85nod@xxxxxxxxx>:
>> > From 16b1d22ae886206def5fc71b26584c329755c81c Mon Sep 17 00:00:00 2001
>> > From: Chih-Yen Chang <cc85nod@xxxxxxxxx>
>> > Date: Thu, 25 May 2023 13:17:27 +0800
>> > Subject: [PATCH] ksmbd: fix out-of-bound read in
>> > deassemble_neg_contexts
>> >
>> > The check in the beginning is
>> > `clen + sizeof(struct smb2_neg_context) <= len_of_ctxts`,
>> > but in the end of loop, `len_of_ctxts` will subtract
>> > `((clen + 7) & ~0x7) + sizeof(struct smb2_neg_context)`, which causes
>> > integer underflow when clen does the 8 alignment.
>> >
>> > [ 11.671070] BUG: KASAN: slab-out-of-bounds in
>> > smb2_handle_negotiate+0x799/0x1610
>> > [ 11.671533] Read of size 2 at addr ffff888005e86cf2 by task
>> > kworker/0:0/7
>> > ...
>> > [ 11.673383] Call Trace:
>> > [ 11.673541] <TASK>
>> > [ 11.673679] dump_stack_lvl+0x33/0x50
>> > [ 11.673913] print_report+0xcc/0x620
>> > [ 11.674671] kasan_report+0xae/0xe0
>> > [ 11.675171] kasan_check_range+0x35/0x1b0
>> > [ 11.675412] smb2_handle_negotiate+0x799/0x1610
>> > [ 11.676217] ksmbd_smb_negotiate_common+0x526/0x770
>> > [ 11.676795] handle_ksmbd_work+0x274/0x810
>> > ...
>> >
>> > Signed-off-by: Chih-Yen Chang <cc85nod@xxxxxxxxx>
>> > ---
>> > fs/ksmbd/smb2pdu.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> > index cb93fd231..2d2a2eb96 100644
>> > --- a/fs/ksmbd/smb2pdu.c
>> > +++ b/fs/ksmbd/smb2pdu.c
>> > @@ -1030,6 +1030,9 @@ static __le32 deassemble_neg_contexts(struct
>> > ksmbd_conn *conn,
>> > clen = (clen + 7) & ~0x7;
>> > offset = clen + sizeof(struct smb2_neg_context);
>> > len_of_ctxts -= clen + sizeof(struct smb2_neg_context);
>> > +
>> > + if (len_of_ctxts < 0)
>> > + break;
>>
>> There is a condition to check len_of_ctxts in this loop. How is this
>> check not making a break ?
>> if (len_of_ctxts < sizeof(struct smb2_neg_context))
>> break;
>> Thanks.
>> > }
>> > return status;
>> > }
>> > --
>> > 2.39.2 (Apple Git-143)
>> >
>>
>
