| Message ID | 20240125214415.27231-2-bethany.jamison@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | CVE-2023-38431 | expand |
On 25.01.24 22:44, Bethany Jamison wrote: > From: Namjae Jeon <linkinjeon@kernel.org> > > The length field of netbios header must be greater than the SMB header > sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB packet. > > If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to `conn->request_buf`. > In the function `get_smb2_cmd_val` ksmbd will read cmd from > `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN > detector to print the following error message: > > [ 7.205018] BUG: KASAN: slab-out-of-bounds in get_smb2_cmd_val+0x45/0x60 > [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task ksmbd:42632/248 > ... > [ 7.207125] <TASK> > [ 7.209191] get_smb2_cmd_val+0x45/0x60 > [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 > [ 7.209712] ksmbd_server_process_request+0x72/0x160 > [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 > [ 7.212280] kthread+0x160/0x190 > [ 7.212762] ret_from_fork+0x1f/0x30 > [ 7.212981] </TASK> > > Cc: stable@vger.kernel.org > Reported-by: Chih-Yen Chang <cc85nod@gmail.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > Signed-off-by: Steve French <stfrench@microsoft.com> > (backported from commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0) > [bjamison: Jammy was missing the definition of smb2_get_msg function which was > used in the fix commit - function definition was very simple so I added it] I do not want to say NACK right away but are you sure this works? The commit which added this functions was commit cb4517201b8acdb5fd5314494aaf86c267f22345 Author: Namjae Jeon <linkinjeon@kernel.org> Date: Wed Nov 3 08:08:44 2021 +0900 ksmbd: remove smb2_buf_length in smb2_hdr Which also did a lot of rework in the structures... When I looks at one of the places where the smb2_get_msg gets added: diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c index f9dae6ef2115..ce0e85552da9 100644 --- a/fs/ksmbd/oplock.c +++ b/fs/ksmbd/oplock.c @@ -629,10 +629,10 @@ static void __smb2_oplock_break_noti(struct work_struct *wk) return; } - rsp_hdr = work->response_buf; + rsp_hdr = smb2_get_msg(work->response_buf); This looks like Jammy code would still expect the rsp_hdr to be where work->reponse_buf points to. While the access function uses a +4 offset. > CVE-2023-38431 > Signed-off-by: Bethany Jamison <bethany.jamison@canonical.com> > --- > fs/ksmbd/connection.c | 12 ++++++++++++ > fs/ksmbd/smb2pdu.h | 9 +++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c > index 3c4b7a96919ce..33d9f6ade7004 100644 > --- a/fs/ksmbd/connection.c > +++ b/fs/ksmbd/connection.c > @@ -263,6 +263,9 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn) > return true; > } > > +#define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr)) > +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4) > + > /** > * ksmbd_conn_handler_loop() - session thread to listen on new smb requests > * @p: connection instance > @@ -319,6 +322,9 @@ int ksmbd_conn_handler_loop(void *p) > if (pdu_size > MAX_STREAM_PROT_LEN) > break; > > + if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE) > + break; > + > /* 4 for rfc1002 length field */ > /* 1 for implied bcc[0] */ > size = pdu_size + 4 + 1; > @@ -346,6 +352,12 @@ int ksmbd_conn_handler_loop(void *p) > continue; > } > > + if (((struct smb2_hdr *)smb2_get_msg(conn->request_buf))->ProtocolId == Maybe that should be if ( ((struct smb2_hdr *) conn->request_buf )->ProtocolId == SMB2_PROTO_NUMBER) { > + SMB2_PROTO_NUMBER) { > + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) > + break; > + } > + > if (!default_conn_ops.process_fn) { > pr_err("No connection request callback\n"); > break; > diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h > index fa1cd556ab7ac..c8cc1083d3c81 100644 > --- a/fs/ksmbd/smb2pdu.h > +++ b/fs/ksmbd/smb2pdu.h > @@ -1707,4 +1707,13 @@ int smb2_ioctl(struct ksmbd_work *work); > int smb2_oplock_break(struct ksmbd_work *work); > int smb2_notify(struct ksmbd_work *ksmbd_work); > > +/* > + * Get the body of the smb2 message excluding the 4 byte rfc1002 headers > + * from request/response buffer. > + */ > +static inline void *smb2_get_msg(void *buf) > +{ > + return buf + 4; > +} > + > #endif /* _SMB2PDU_H */
On 26.01.24 10:55, Stefan Bader wrote: > On 25.01.24 22:44, Bethany Jamison wrote: >> From: Namjae Jeon <linkinjeon@kernel.org> >> >> The length field of netbios header must be greater than the SMB header >> sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB >> packet. >> >> If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to >> `conn->request_buf`. >> In the function `get_smb2_cmd_val` ksmbd will read cmd from >> `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN >> detector to print the following error message: >> >> [ 7.205018] BUG: KASAN: slab-out-of-bounds in >> get_smb2_cmd_val+0x45/0x60 >> [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task >> ksmbd:42632/248 >> ... >> [ 7.207125] <TASK> >> [ 7.209191] get_smb2_cmd_val+0x45/0x60 >> [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 >> [ 7.209712] ksmbd_server_process_request+0x72/0x160 >> [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 >> [ 7.212280] kthread+0x160/0x190 >> [ 7.212762] ret_from_fork+0x1f/0x30 >> [ 7.212981] </TASK> >> >> Cc: stable@vger.kernel.org >> Reported-by: Chih-Yen Chang <cc85nod@gmail.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> Signed-off-by: Steve French <stfrench@microsoft.com> >> (backported from commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0) >> [bjamison: Jammy was missing the definition of smb2_get_msg function >> which was >> used in the fix commit - function definition was very simple so I >> added it] > > I do not want to say NACK right away but are you sure this works? The > commit which added this functions was > > commit cb4517201b8acdb5fd5314494aaf86c267f22345 > Author: Namjae Jeon <linkinjeon@kernel.org> > Date: Wed Nov 3 08:08:44 2021 +0900 > > ksmbd: remove smb2_buf_length in smb2_hdr > > Which also did a lot of rework in the structures... When I looks at one > of the places where the smb2_get_msg gets added: > > diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c > index f9dae6ef2115..ce0e85552da9 100644 > --- a/fs/ksmbd/oplock.c > +++ b/fs/ksmbd/oplock.c > @@ -629,10 +629,10 @@ static void __smb2_oplock_break_noti(struct > work_struct *wk) > return; > } > > - rsp_hdr = work->response_buf; > + rsp_hdr = smb2_get_msg(work->response_buf); > > This looks like Jammy code would still expect the rsp_hdr to be where > work->reponse_buf points to. While the access function uses a +4 offset. > Hi Bethany, this request is in a bit of a fuzzy state. Did we figure out whether my concern was valid or not. If yes, could you nack this, if no, maybe you could explain me why I am wrong. > >> CVE-2023-38431 >> Signed-off-by: Bethany Jamison <bethany.jamison@canonical.com> >> --- >> fs/ksmbd/connection.c | 12 ++++++++++++ >> fs/ksmbd/smb2pdu.h | 9 +++++++++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c >> index 3c4b7a96919ce..33d9f6ade7004 100644 >> --- a/fs/ksmbd/connection.c >> +++ b/fs/ksmbd/connection.c >> @@ -263,6 +263,9 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn) >> return true; >> } >> +#define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr)) >> +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4) >> + >> /** >> * ksmbd_conn_handler_loop() - session thread to listen on new smb >> requests >> * @p: connection instance >> @@ -319,6 +322,9 @@ int ksmbd_conn_handler_loop(void *p) >> if (pdu_size > MAX_STREAM_PROT_LEN) >> break; >> + if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE) >> + break; >> + >> /* 4 for rfc1002 length field */ >> /* 1 for implied bcc[0] */ >> size = pdu_size + 4 + 1; >> @@ -346,6 +352,12 @@ int ksmbd_conn_handler_loop(void *p) >> continue; >> } >> + if (((struct smb2_hdr >> *)smb2_get_msg(conn->request_buf))->ProtocolId == > > Maybe that should be > > if ( ((struct smb2_hdr *) conn->request_buf )->ProtocolId == > SMB2_PROTO_NUMBER) { > > >> + SMB2_PROTO_NUMBER) { >> + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) >> + break; >> + } >> + >> if (!default_conn_ops.process_fn) { >> pr_err("No connection request callback\n"); >> break; >> diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h >> index fa1cd556ab7ac..c8cc1083d3c81 100644 >> --- a/fs/ksmbd/smb2pdu.h >> +++ b/fs/ksmbd/smb2pdu.h >> @@ -1707,4 +1707,13 @@ int smb2_ioctl(struct ksmbd_work *work); >> int smb2_oplock_break(struct ksmbd_work *work); >> int smb2_notify(struct ksmbd_work *ksmbd_work); >> +/* >> + * Get the body of the smb2 message excluding the 4 byte rfc1002 headers >> + * from request/response buffer. >> + */ >> +static inline void *smb2_get_msg(void *buf) >> +{ >> + return buf + 4; >> +} >> + >> #endif /* _SMB2PDU_H */ > > Thanks, - Stefan
On 13/02/2024 10:36, Stefan Bader wrote: > On 26.01.24 10:55, Stefan Bader wrote: >> On 25.01.24 22:44, Bethany Jamison wrote: >>> From: Namjae Jeon <linkinjeon@kernel.org> >>> >>> The length field of netbios header must be greater than the SMB header >>> sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB >>> packet. >>> >>> If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to >>> `conn->request_buf`. >>> In the function `get_smb2_cmd_val` ksmbd will read cmd from >>> `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the >>> KASAN >>> detector to print the following error message: >>> >>> [ 7.205018] BUG: KASAN: slab-out-of-bounds in >>> get_smb2_cmd_val+0x45/0x60 >>> [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task >>> ksmbd:42632/248 >>> ... >>> [ 7.207125] <TASK> >>> [ 7.209191] get_smb2_cmd_val+0x45/0x60 >>> [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 >>> [ 7.209712] ksmbd_server_process_request+0x72/0x160 >>> [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 >>> [ 7.212280] kthread+0x160/0x190 >>> [ 7.212762] ret_from_fork+0x1f/0x30 >>> [ 7.212981] </TASK> >>> >>> Cc: stable@vger.kernel.org >>> Reported-by: Chih-Yen Chang <cc85nod@gmail.com> >>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>> Signed-off-by: Steve French <stfrench@microsoft.com> >>> (backported from commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0) >>> [bjamison: Jammy was missing the definition of smb2_get_msg function >>> which was >>> used in the fix commit - function definition was very simple so I >>> added it] >> >> I do not want to say NACK right away but are you sure this works? The >> commit which added this functions was >> >> commit cb4517201b8acdb5fd5314494aaf86c267f22345 >> Author: Namjae Jeon <linkinjeon@kernel.org> >> Date: Wed Nov 3 08:08:44 2021 +0900 >> >> ksmbd: remove smb2_buf_length in smb2_hdr >> >> Which also did a lot of rework in the structures... When I looks at >> one of the places where the smb2_get_msg gets added: >> >> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c >> index f9dae6ef2115..ce0e85552da9 100644 >> --- a/fs/ksmbd/oplock.c >> +++ b/fs/ksmbd/oplock.c >> @@ -629,10 +629,10 @@ static void __smb2_oplock_break_noti(struct >> work_struct *wk) >> return; >> } >> >> - rsp_hdr = work->response_buf; >> + rsp_hdr = smb2_get_msg(work->response_buf); >> >> This looks like Jammy code would still expect the rsp_hdr to be where >> work->reponse_buf points to. While the access function uses a +4 offset. >> > > Hi Bethany, > > this request is in a bit of a fuzzy state. Did we figure out whether > my concern was valid or not. If yes, could you nack this, if no, maybe > you could explain me why I am wrong. >> >>> CVE-2023-38431 >>> Signed-off-by: Bethany Jamison <bethany.jamison@canonical.com> >>> --- >>> fs/ksmbd/connection.c | 12 ++++++++++++ >>> fs/ksmbd/smb2pdu.h | 9 +++++++++ >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c >>> index 3c4b7a96919ce..33d9f6ade7004 100644 >>> --- a/fs/ksmbd/connection.c >>> +++ b/fs/ksmbd/connection.c >>> @@ -263,6 +263,9 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn) >>> return true; >>> } >>> +#define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr)) >>> +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4) >>> + >>> /** >>> * ksmbd_conn_handler_loop() - session thread to listen on new smb >>> requests >>> * @p: connection instance >>> @@ -319,6 +322,9 @@ int ksmbd_conn_handler_loop(void *p) >>> if (pdu_size > MAX_STREAM_PROT_LEN) >>> break; >>> + if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE) >>> + break; >>> + >>> /* 4 for rfc1002 length field */ >>> /* 1 for implied bcc[0] */ >>> size = pdu_size + 4 + 1; >>> @@ -346,6 +352,12 @@ int ksmbd_conn_handler_loop(void *p) >>> continue; >>> } >>> + if (((struct smb2_hdr >>> *)smb2_get_msg(conn->request_buf))->ProtocolId == >> >> Maybe that should be >> >> if ( ((struct smb2_hdr *) conn->request_buf )->ProtocolId == >> SMB2_PROTO_NUMBER) { >> >> >>> + SMB2_PROTO_NUMBER) { >>> + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) >>> + break; >>> + } >>> + >>> if (!default_conn_ops.process_fn) { >>> pr_err("No connection request callback\n"); >>> break; >>> diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h >>> index fa1cd556ab7ac..c8cc1083d3c81 100644 >>> --- a/fs/ksmbd/smb2pdu.h >>> +++ b/fs/ksmbd/smb2pdu.h >>> @@ -1707,4 +1707,13 @@ int smb2_ioctl(struct ksmbd_work *work); >>> int smb2_oplock_break(struct ksmbd_work *work); >>> int smb2_notify(struct ksmbd_work *ksmbd_work); >>> +/* >>> + * Get the body of the smb2 message excluding the 4 byte rfc1002 >>> headers >>> + * from request/response buffer. >>> + */ >>> +static inline void *smb2_get_msg(void *buf) >>> +{ >>> + return buf + 4; >>> +} >>> + >>> #endif /* _SMB2PDU_H */ >> >> > Thanks, > - Stefan > > Hi Bethany, Could you address Stefan's concerns? This patch has been sitting here for a while. Roxana
diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c index 3c4b7a96919ce..33d9f6ade7004 100644 --- a/fs/ksmbd/connection.c +++ b/fs/ksmbd/connection.c @@ -263,6 +263,9 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn) return true; } +#define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr)) +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4) + /** * ksmbd_conn_handler_loop() - session thread to listen on new smb requests * @p: connection instance @@ -319,6 +322,9 @@ int ksmbd_conn_handler_loop(void *p) if (pdu_size > MAX_STREAM_PROT_LEN) break; + if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE) + break; + /* 4 for rfc1002 length field */ /* 1 for implied bcc[0] */ size = pdu_size + 4 + 1; @@ -346,6 +352,12 @@ int ksmbd_conn_handler_loop(void *p) continue; } + if (((struct smb2_hdr *)smb2_get_msg(conn->request_buf))->ProtocolId == + SMB2_PROTO_NUMBER) { + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) + break; + } + if (!default_conn_ops.process_fn) { pr_err("No connection request callback\n"); break; diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h index fa1cd556ab7ac..c8cc1083d3c81 100644 --- a/fs/ksmbd/smb2pdu.h +++ b/fs/ksmbd/smb2pdu.h @@ -1707,4 +1707,13 @@ int smb2_ioctl(struct ksmbd_work *work); int smb2_oplock_break(struct ksmbd_work *work); int smb2_notify(struct ksmbd_work *ksmbd_work); +/* + * Get the body of the smb2 message excluding the 4 byte rfc1002 headers + * from request/response buffer. + */ +static inline void *smb2_get_msg(void *buf) +{ + return buf + 4; +} + #endif /* _SMB2PDU_H */