| Message ID | 20230517095951.3476020-1-h3xrabbit@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate | expand |
2023-05-17 18:59 GMT+09:00, HexRabbit <h3xrabbit@gmail.com>: > Check request_buf length first to avoid out-of-bounds read by > req->DialectCount. > > [ 3350.990282] BUG: KASAN: slab-out-of-bounds in > smb2_handle_negotiate+0x35d7/0x3e60 > [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task > kworker/5:0/276 > [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work > [ 3351.003499] Call Trace: > [ 3351.006473] <TASK> > [ 3351.006473] dump_stack_lvl+0x8d/0xe0 > [ 3351.006473] print_report+0xcc/0x620 > [ 3351.006473] kasan_report+0x92/0xc0 > [ 3351.006473] smb2_handle_negotiate+0x35d7/0x3e60 > [ 3351.014760] ksmbd_smb_negotiate_common+0x7a7/0xf00 > [ 3351.014760] handle_ksmbd_work+0x3f7/0x12d0 > [ 3351.014760] process_one_work+0xa85/0x1780 > > Signed-off-by: HexRabbit <h3xrabbit@gmail.com> Acked-by: Namjae Jeon <linkinjeon@kernel.org> Sergey will say, "Do we still have a requirement that there should be a real name in SoB?" Thanks for your patch!
On (23/05/17 19:17), Namjae Jeon wrote: > > Signed-off-by: HexRabbit <h3xrabbit@gmail.com> > Acked-by: Namjae Jeon <linkinjeon@kernel.org> > > Sergey will say, "Do we still have a requirement that there should be > a real name in SoB?" :) Thanks!
On (23/05/17 09:59), HexRabbit wrote: > [ 3350.990282] BUG: KASAN: slab-out-of-bounds in smb2_handle_negotiate+0x35d7/0x3e60 > [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task kworker/5:0/276 > [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work > [ 3351.003499] Call Trace: > [ 3351.006473] <TASK> > [ 3351.006473] dump_stack_lvl+0x8d/0xe0 > [ 3351.006473] print_report+0xcc/0x620 > [ 3351.006473] kasan_report+0x92/0xc0 > [ 3351.006473] smb2_handle_negotiate+0x35d7/0x3e60 > [ 3351.014760] ksmbd_smb_negotiate_common+0x7a7/0xf00 > [ 3351.014760] handle_ksmbd_work+0x3f7/0x12d0 > [ 3351.014760] process_one_work+0xa85/0x1780 [..] > - if (req->DialectCount == 0) { > - pr_err("malformed packet\n"); > + smb2_buf_len = get_rfc1002_len(work->request_buf); > + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); > + if (smb2_neg_size > smb2_buf_len) { > rsp->hdr.Status = STATUS_INVALID_PARAMETER; > rc = -EINVAL; > goto err_out; > } > > - smb2_buf_len = get_rfc1002_len(work->request_buf); > - smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); > - if (smb2_neg_size > smb2_buf_len) { > + if (req->DialectCount == 0) { > + pr_err("malformed packet\n"); > rsp->hdr.Status = STATUS_INVALID_PARAMETER; > rc = -EINVAL; > goto err_out; May I please ask where out-of-bounds access happens and how does `smb2_neg_size > smb2_buf_len` fix it?
On (23/05/17 20:05), Sergey Senozhatsky wrote: > On (23/05/17 09:59), HexRabbit wrote: > > [ 3350.990282] BUG: KASAN: slab-out-of-bounds in smb2_handle_negotiate+0x35d7/0x3e60 > > [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task kworker/5:0/276 > > [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work > > [ 3351.003499] Call Trace: > > [ 3351.006473] <TASK> > > [ 3351.006473] dump_stack_lvl+0x8d/0xe0 > > [ 3351.006473] print_report+0xcc/0x620 > > [ 3351.006473] kasan_report+0x92/0xc0 > > [ 3351.006473] smb2_handle_negotiate+0x35d7/0x3e60 > > [ 3351.014760] ksmbd_smb_negotiate_common+0x7a7/0xf00 > > [ 3351.014760] handle_ksmbd_work+0x3f7/0x12d0 > > [ 3351.014760] process_one_work+0xa85/0x1780 > > [..] > > > - if (req->DialectCount == 0) { > > - pr_err("malformed packet\n"); > > + smb2_buf_len = get_rfc1002_len(work->request_buf); > > + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); > > + if (smb2_neg_size > smb2_buf_len) { > > rsp->hdr.Status = STATUS_INVALID_PARAMETER; > > rc = -EINVAL; > > goto err_out; > > } > > > > - smb2_buf_len = get_rfc1002_len(work->request_buf); > > - smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); > > - if (smb2_neg_size > smb2_buf_len) { > > + if (req->DialectCount == 0) { > > + pr_err("malformed packet\n"); > > rsp->hdr.Status = STATUS_INVALID_PARAMETER; > > rc = -EINVAL; > > goto err_out; > > May I please ask where out-of-bounds access happens and how does > `smb2_neg_size > smb2_buf_len` fix it? Correction: I meant to ask "how does moving `smb2_neg_size > smb2_buf_len` up fixes it?". We have this in the code at the moment ``` if (req->DialectCount == 0) { pr_err("malformed packet\n"); rsp->hdr.Status = STATUS_INVALID_PARAMETER; rc = -EINVAL; goto err_out; } smb2_buf_len = get_rfc1002_len(work->request_buf); smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); if (smb2_neg_size > smb2_buf_len) { rsp->hdr.Status = STATUS_INVALID_PARAMETER; rc = -EINVAL; goto err_out; } ``` But if we move `smb2_neg_size > smb2_buf_len` brunch up, then it cures out-of-bounds access? Where is that out-of-bounds access? Looking at the stack trace, smb2_handle_negotiate+0x35d7/0x3e60 should be somewhere much-much later than these if-s.
On (23/05/17 19:45), Hex Rabbit wrote: > The out-of-bounds access is triggered by `req->DialectCount` memory > access, > sender can construct a malformed packet that only has a single > `smb2_negotiate_req.StructureSize` field after the smb2 header. Oh, I see, is that what you did in your reproducer? > Referring to the payload I showed below, since the function is > assuming that the entire `smb2_negotiate_req` structure is presented, > it will read above the `StructureSize` field (2400) and trigger KASAN. > So check against `smb2_buf_len` first will ensure entire structure > is inside the buffer, hope this make sense! > ``` > 00000000: 0000 0042 fe53 4d42 4000 0000 0000 0000 ...B.SMB@....... > 00000010: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > 00000020: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > 00000030: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > 00000040: 0000 0000 2400 > ....$. > ``` > sorry for not providing full symbolized stack trace first Thanks for clarifications. Maybe would be nice to have some of these lines in the commit message. > Call Trace: > dump_stack_lvl (lib/dump_stack.c:107) > print_report (mm/kasan/report.c:352 mm/kasan/report.c:462) > kasan_report (mm/kasan/report.c:574) > smb2_handle_negotiate (fs/ksmbd/smb2pdu.c:1060) I'm still puzzled by smb2_handle_negotiate+0x35d7/0x3e60 in the original stack trace. 0x35d7/0x3e60 certainly doesn't translate to "start of the function" to me, but what do I know :)
> Oh, I see, is that what you did in your reproducer? Yes, that's how I reproduce it. > I'm still puzzled by smb2_handle_negotiate+0x35d7/0x3e60 in the original > stack trace. 0x35d7/0x3e60 certainly doesn't translate to "start of the > function" to me, but what do I know :) As you can see in the assembly below, the call to asan_report_* functions is placed at the bottom of the function, that's why the stack trace looks like that. ``` ; smb2_handle_negotiate+0x216 .text:FFFFFFFF81FDA4F6 test dl, dl .text:FFFFFFFF81FDA4F8 setnz al .text:FFFFFFFF81FDA4FB test cl, al ; KASAN check .text:FFFFFFFF81FDA4FD jnz loc_FFFFFFFF81FDD80E ; jump to report ; smb2_handle_negotiate+0x352e .text:FFFFFFFF81FDD80E loc_FFFFFFFF81FDD80E: .text:FFFFFFFF81FDD80E mov esi, 2 .text:FFFFFFFF81FDD813 call __asan_report_load_n_noabort .text:FFFFFFFF81FDD818 jmp loc_FFFFFFFF81FDA503 .text:FFFFFFFF81FDD81D loc_FFFFFFFF81FDD81D: .text:FFFFFFFF81FDD81D mov esi, 4 .text:FFFFFFFF81FDD822 call __asan_report_store_n_noabort .text:FFFFFFFF81FDD827 jmp loc_FFFFFFFF81FDAFA7 .text:FFFFFFFF81FDD82C loc_FFFFFFFF81FDD82C: .text:FFFFFFFF81FDD82C mov rdi, rcx .text:FFFFFFFF81FDD82F call __asan_report_load2_noabort .text:FFFFFFFF81FDD834 jmp loc_FFFFFFFF81FDA558 ```
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index cb93fd231..972176bff 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -1057,16 +1057,16 @@ int smb2_handle_negotiate(struct ksmbd_work *work) return rc; } - if (req->DialectCount == 0) { - pr_err("malformed packet\n"); + smb2_buf_len = get_rfc1002_len(work->request_buf); + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); + if (smb2_neg_size > smb2_buf_len) { rsp->hdr.Status = STATUS_INVALID_PARAMETER; rc = -EINVAL; goto err_out; } - smb2_buf_len = get_rfc1002_len(work->request_buf); - smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects); - if (smb2_neg_size > smb2_buf_len) { + if (req->DialectCount == 0) { + pr_err("malformed packet\n"); rsp->hdr.Status = STATUS_INVALID_PARAMETER; rc = -EINVAL; goto err_out;
Check request_buf length first to avoid out-of-bounds read by req->DialectCount. [ 3350.990282] BUG: KASAN: slab-out-of-bounds in smb2_handle_negotiate+0x35d7/0x3e60 [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task kworker/5:0/276 [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work [ 3351.003499] Call Trace: [ 3351.006473] <TASK> [ 3351.006473] dump_stack_lvl+0x8d/0xe0 [ 3351.006473] print_report+0xcc/0x620 [ 3351.006473] kasan_report+0x92/0xc0 [ 3351.006473] smb2_handle_negotiate+0x35d7/0x3e60 [ 3351.014760] ksmbd_smb_negotiate_common+0x7a7/0xf00 [ 3351.014760] handle_ksmbd_work+0x3f7/0x12d0 [ 3351.014760] process_one_work+0xa85/0x1780 Signed-off-by: HexRabbit <h3xrabbit@gmail.com> --- fs/ksmbd/smb2pdu.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)