| Message ID | 20210923034855.612832-2-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] ksmbd: fix invalid request buffer access in compound | expand |
On 9/22/2021 11:48 PM, Namjae Jeon wrote: > Ronnie reported invalid request buffer access in chained command when > inserting garbage value to NextCommand of compound request. > This patch add validation check to avoid this issue. > > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Reported-by: Ronnie Sahlberg <lsahlber@redhat.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > v2: > - fix integer overflow from work->next_smb2_rcv_hdr_off. > v3: > - check next command offset and at least header size of next pdu at > the same time. > v4: > - add next_cmd variable not to avoid repeat conversion. > > fs/ksmbd/smb2pdu.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index 90f867b9d560..301558a04298 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work) > bool is_chained_smb2_message(struct ksmbd_work *work) > { > struct smb2_hdr *hdr = work->request_buf; > - unsigned int len; > + unsigned int len, next_cmd; > > if (hdr->ProtocolId != SMB2_PROTO_NUMBER) > return false; > > hdr = ksmbd_req_buf_next(work); > - if (le32_to_cpu(hdr->NextCommand) > 0) { > + next_cmd = le32_to_cpu(hdr->NextCommand); > + if (next_cmd > 0) { > + if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 > The "64" is somewhat arbitrary and mysterious. Is this the size of the next command smb2_hdr? Why not code that, at least with a #define? > + get_rfc1002_len(work->request_buf)) { > + pr_err("next command(%u) offset exceeds smb msg size\n", > + next_cmd); > + return false; > + } Hmm, well the header fits in the reminder of the message. What about the rest of the nextcommand smb2 request? It seems very odd, and more than a little risky, to be validating only one aspect of the nextcommand here. > + > ksmbd_debug(SMB, "got SMB2 chained command\n"); This, to me, is entirely needless debug splat. Tom. > init_chained_smb2_rsp(work); > return true; >
2021-09-24 0:13 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/22/2021 11:48 PM, Namjae Jeon wrote: >> Ronnie reported invalid request buffer access in chained command when >> inserting garbage value to NextCommand of compound request. >> This patch add validation check to avoid this issue. >> >> Cc: Tom Talpey <tom@talpey.com> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Ralph Böhme <slow@samba.org> >> Cc: Steve French <smfrench@gmail.com> >> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> v2: >> - fix integer overflow from work->next_smb2_rcv_hdr_off. >> v3: >> - check next command offset and at least header size of next pdu at >> the same time. >> v4: >> - add next_cmd variable not to avoid repeat conversion. >> >> fs/ksmbd/smb2pdu.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index 90f867b9d560..301558a04298 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work >> *work) >> bool is_chained_smb2_message(struct ksmbd_work *work) >> { >> struct smb2_hdr *hdr = work->request_buf; >> - unsigned int len; >> + unsigned int len, next_cmd; >> >> if (hdr->ProtocolId != SMB2_PROTO_NUMBER) >> return false; >> >> hdr = ksmbd_req_buf_next(work); >> - if (le32_to_cpu(hdr->NextCommand) > 0) { >> + next_cmd = le32_to_cpu(hdr->NextCommand); >> + if (next_cmd > 0) { >> + if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 > > > The "64" is somewhat arbitrary and mysterious. Is this the size > of the next command smb2_hdr? Why not code that, at least with > a #define? Okay, Will use macro. > >> + get_rfc1002_len(work->request_buf)) { >> + pr_err("next command(%u) offset exceeds smb msg size\n", >> + next_cmd); >> + return false; >> + } > > Hmm, well the header fits in the reminder of the message. What > about the rest of the nextcommand smb2 request? It seems very > odd, and more than a little risky, to be validating only one > aspect of the nextcommand here. There is a loop to check the rest of the nextcommand. Please see do { } while (is_chained_smb2_message(work)); in server. > >> + >> ksmbd_debug(SMB, "got SMB2 chained command\n"); > > This, to me, is entirely needless debug splat. The reason is ? > > Tom. > >> init_chained_smb2_rsp(work); >> return true; >> >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index 90f867b9d560..301558a04298 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work) bool is_chained_smb2_message(struct ksmbd_work *work) { struct smb2_hdr *hdr = work->request_buf; - unsigned int len; + unsigned int len, next_cmd; if (hdr->ProtocolId != SMB2_PROTO_NUMBER) return false; hdr = ksmbd_req_buf_next(work); - if (le32_to_cpu(hdr->NextCommand) > 0) { + next_cmd = le32_to_cpu(hdr->NextCommand); + if (next_cmd > 0) { + if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 > + get_rfc1002_len(work->request_buf)) { + pr_err("next command(%u) offset exceeds smb msg size\n", + next_cmd); + return false; + } + ksmbd_debug(SMB, "got SMB2 chained command\n"); init_chained_smb2_rsp(work); return true;
Ronnie reported invalid request buffer access in chained command when inserting garbage value to NextCommand of compound request. This patch add validation check to avoid this issue. Cc: Tom Talpey <tom@talpey.com> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Ralph Böhme <slow@samba.org> Cc: Steve French <smfrench@gmail.com> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- v2: - fix integer overflow from work->next_smb2_rcv_hdr_off. v3: - check next command offset and at least header size of next pdu at the same time. v4: - add next_cmd variable not to avoid repeat conversion. fs/ksmbd/smb2pdu.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)