2023-05-26 11:56 GMT+09:00, 張智諺 <cc85nod@xxxxxxxxx>:
> Hello, Namjae Jeon,
>
> This is my new patch, and I just updated the check to make sure
> `len_of_ctxts` is still large enough after `clen` alignment.
> I also updated the types of some variables that Tom Talpey mentioned
> yesterday.
pctx = (struct smb2_neg_context *)((char *)pctx + offset);
clen = le16_to_cpu(pctx->DataLength);
len_of_ctxts should be checked before accessing pctx and pctx-DataLength ?
- if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
+ if (((clen + 7) & ~0x7) + sizeof(struct
smb2_neg_context) > len_of_ctxts)
break;
This patch should be created after applying "ksmbd: fix multiple
out-of-bounds read during context decoding" patch".
>
> Thanks.
>
> Namjae Jeon <linkinjeon@xxxxxxxxxx> 於 2023年5月25日 週四 下午8:58寫道：
>
>> 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)
>> >> >
>> >>
>> >
>>
>
