On 5/25/2023 8:58 AM, Namjae Jeon wrote:
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 ?
The len_of_ctxts should not be signed. There will be some necessary
overflow checks, to replace the incorrect underflow checks of course.
I'll observe that the len_of_smb, offset and neg_ctxt_cnt in the
same function are similarly incorrectly typed.
Tom.
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)
