| Message ID | 20210921225109.6388-3-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] ksmbd: remove RFC1002 check in smb2 request | expand |
On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> 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: 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. > > fs/ksmbd/smb2pdu.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index 1fe37ad4e5bc..cae796ea1148 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work *work) > > hdr = ksmbd_req_buf_next(work); > if (le32_to_cpu(hdr->NextCommand) > 0) { > + if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) > > + get_rfc1002_len(work->request_buf)) { > + pr_err("next command(%u) offset exceeds smb msg size\n", > + hdr->NextCommand); > + return false; > + } > + > ksmbd_debug(SMB, "got SMB2 chained command\n"); > init_chained_smb2_rsp(work); > return true; Very good, reviewed by me. The conditional though, since you know there will be at least a full smb2 header there you could already check that change it to > + if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) > > + get_rfc1002_len(work->request_buf) + 64) { Which leads to another question. Where do you check that the buffer contains enough data to hold the smb2 header and the full fixed part of the request? There is a check that you have enough space for the smb2 header in ksmbd_conn_handler_loop() that there is enough space for the smb2 header (ksmbd_pdu_size_has_room()) but that function assumes that the smb2 header always start at the head of the buffer. So if you have a compound chain, this functrion only checks the first pdu. I know that the buffer handling is copied from the cifs client. It used to also do these "just pass a buffer around and the first 4 bytes is the size" (and still does for smb1) and there was a lot of terrible +4 or -4 to all sort of casts and conditionals. I changed that in cifs.ko to remove the 4 byte length completely from the buffer. I also changed it as part of the compounding to pass an array of requests (each containing an iovector) to the functions instead of just one large byte array. That made things a lot easier to manage since you could then assume that the SMB2 header would always start at offset 0 in the corresponding iovector, even for compounded commands since they all had their own private vector. And since an iovector contains both a pointer and a length there is no need anymore to read the first 4 bytes/validate them/and covnert into a length all the time. I think that would help, but it would be a MAJOR amount of work, so maybe that should wait until later. That approach is very nice since it completely avoids keeping track of offset-to-where-this-pdu-starts which makes all checks and conditionals so much more complex. regards ronnie sahlberg > -- > 2.25.1 >
2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: > On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> 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: 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. >> >> fs/ksmbd/smb2pdu.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index 1fe37ad4e5bc..cae796ea1148 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work >> *work) >> >> hdr = ksmbd_req_buf_next(work); >> if (le32_to_cpu(hdr->NextCommand) > 0) { >> + if ((u64)work->next_smb2_rcv_hdr_off + >> le32_to_cpu(hdr->NextCommand) > >> + get_rfc1002_len(work->request_buf)) { >> + pr_err("next command(%u) offset exceeds smb msg >> size\n", >> + hdr->NextCommand); >> + return false; >> + } >> + >> ksmbd_debug(SMB, "got SMB2 chained command\n"); >> init_chained_smb2_rsp(work); >> return true; > > Very good, reviewed by me. Sorry for late response, Thanks for your review! > The conditional though, since you know there will be at least a full > smb2 header there you could already check that change it to >> + if ((u64)work->next_smb2_rcv_hdr_off + >> le32_to_cpu(hdr->NextCommand) > >> + get_rfc1002_len(work->request_buf) + 64) { Ah, I didn't understand why we should add + 64(smb2 hdr size)... As I know, NextCommand offset included smb2 header size.. > > > Which leads to another question. Where do you check that the buffer > contains enough data to hold the smb2 header and the full fixed part > of the request? ksmbd_smb2_check_message() in smb2misc.c should check it. > There is a check that you have enough space for the smb2 header in > ksmbd_conn_handler_loop() > that there is enough space for the smb2 header > (ksmbd_pdu_size_has_room()) but that function assumes that the smb2 > header always start at the head of the buffer. > So if you have a compound chain, this functrion only checks the first pdu. I think that is_chained_smb2_message() will check all pdu as well as first pdu. there is loop do { } while (is_chained_smb2_message(work)); in server.c > > > I know that the buffer handling is copied from the cifs client. It > used to also do these "just pass a buffer around and the first 4 bytes > is the size" (and still does for smb1) and there was a lot of > terrible +4 or -4 to all sort of casts and conditionals. > I changed that in cifs.ko to remove the 4 byte length completely from > the buffer. > I also changed it as part of the compounding to pass an array of > requests (each containing an iovector) to the functions instead of > just one large byte array. > That made things a lot easier to manage since you could then assume > that the SMB2 header would always start at offset 0 in the > corresponding iovector, even for compounded commands since they all > had their own private vector. > And since an iovector contains both a pointer and a length there is no > need anymore to read the first 4 bytes/validate them/and covnert into > a length all the time. Right. fully agreed. > > I think that would help, but it would be a MAJOR amount of work, so > maybe that should wait until later. Agreed, I will do that after fixing current urgent issues first! > That approach is very nice since it completely avoids keeping track of > offset-to-where-this-pdu-starts which makes all checks and > conditionals so much more complex. Thanks! > > > regards > ronnie sahlberg > > >> -- >> 2.25.1 >> >
On Wed, Sep 22, 2021 at 2:35 PM Namjae Jeon <linkinjeon@kernel.org> wrote: > > 2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: > > On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> 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: 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. > >> > >> fs/ksmbd/smb2pdu.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> index 1fe37ad4e5bc..cae796ea1148 100644 > >> --- a/fs/ksmbd/smb2pdu.c > >> +++ b/fs/ksmbd/smb2pdu.c > >> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work > >> *work) > >> > >> hdr = ksmbd_req_buf_next(work); > >> if (le32_to_cpu(hdr->NextCommand) > 0) { > >> + if ((u64)work->next_smb2_rcv_hdr_off + > >> le32_to_cpu(hdr->NextCommand) > > >> + get_rfc1002_len(work->request_buf)) { > >> + pr_err("next command(%u) offset exceeds smb msg > >> size\n", > >> + hdr->NextCommand); > >> + return false; > >> + } > >> + > >> ksmbd_debug(SMB, "got SMB2 chained command\n"); > >> init_chained_smb2_rsp(work); > >> return true; > > > > Very good, reviewed by me. > Sorry for late response, Thanks for your review! > > The conditional though, since you know there will be at least a full > > smb2 header there you could already check that change it to > >> + if ((u64)work->next_smb2_rcv_hdr_off + > >> le32_to_cpu(hdr->NextCommand) > > >> + get_rfc1002_len(work->request_buf) + 64) { > Ah, I didn't understand why we should add + 64(smb2 hdr size)... > As I know, NextCommand offset included smb2 header size.. This is what I meant. + if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) + 64 > + get_rfc1002_len(work->request_buf)) { It could just be an early check that what hdr->NextCommand points to has at least 64 bytes. I.e. an early test that "does the next PDU have at least a full smb2 header?" I mean, since you already test that NextCommand is valid, you could at the same time also test that the next pdu is at least 64 bytes. > > > > > > Which leads to another question. Where do you check that the buffer > > contains enough data to hold the smb2 header and the full fixed part > > of the request? > ksmbd_smb2_check_message() in smb2misc.c should check it. > > > There is a check that you have enough space for the smb2 header in > > ksmbd_conn_handler_loop() > > that there is enough space for the smb2 header > > (ksmbd_pdu_size_has_room()) but that function assumes that the smb2 > > header always start at the head of the buffer. > > So if you have a compound chain, this functrion only checks the first pdu. > I think that is_chained_smb2_message() will check all pdu as well as first pdu. > there is loop do { } while (is_chained_smb2_message(work)); in server.c > > > > > > I know that the buffer handling is copied from the cifs client. It > > used to also do these "just pass a buffer around and the first 4 bytes > > is the size" (and still does for smb1) and there was a lot of > > terrible +4 or -4 to all sort of casts and conditionals. > > I changed that in cifs.ko to remove the 4 byte length completely from > > the buffer. > > I also changed it as part of the compounding to pass an array of > > requests (each containing an iovector) to the functions instead of > > just one large byte array. > > That made things a lot easier to manage since you could then assume > > that the SMB2 header would always start at offset 0 in the > > corresponding iovector, even for compounded commands since they all > > had their own private vector. > > And since an iovector contains both a pointer and a length there is no > > need anymore to read the first 4 bytes/validate them/and covnert into > > a length all the time. > Right. fully agreed. > > > > > I think that would help, but it would be a MAJOR amount of work, so > > maybe that should wait until later. > Agreed, I will do that after fixing current urgent issues first! > > > That approach is very nice since it completely avoids keeping track of > > offset-to-where-this-pdu-starts which makes all checks and > > conditionals so much more complex. > Thanks! > > > > > > regards > > ronnie sahlberg > > > > > >> -- > >> 2.25.1 > >> > >
2021-09-22 13:56 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: > On Wed, Sep 22, 2021 at 2:35 PM Namjae Jeon <linkinjeon@kernel.org> wrote: >> >> 2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: >> > On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> >> > 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: 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. >> >> >> >> fs/ksmbd/smb2pdu.c | 7 +++++++ >> >> 1 file changed, 7 insertions(+) >> >> >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> >> index 1fe37ad4e5bc..cae796ea1148 100644 >> >> --- a/fs/ksmbd/smb2pdu.c >> >> +++ b/fs/ksmbd/smb2pdu.c >> >> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work >> >> *work) >> >> >> >> hdr = ksmbd_req_buf_next(work); >> >> if (le32_to_cpu(hdr->NextCommand) > 0) { >> >> + if ((u64)work->next_smb2_rcv_hdr_off + >> >> le32_to_cpu(hdr->NextCommand) > >> >> + get_rfc1002_len(work->request_buf)) { >> >> + pr_err("next command(%u) offset exceeds smb >> >> msg >> >> size\n", >> >> + hdr->NextCommand); >> >> + return false; >> >> + } >> >> + >> >> ksmbd_debug(SMB, "got SMB2 chained command\n"); >> >> init_chained_smb2_rsp(work); >> >> return true; >> > >> > Very good, reviewed by me. >> Sorry for late response, Thanks for your review! >> > The conditional though, since you know there will be at least a full >> > smb2 header there you could already check that change it to >> >> + if ((u64)work->next_smb2_rcv_hdr_off + >> >> le32_to_cpu(hdr->NextCommand) > >> >> + get_rfc1002_len(work->request_buf) + 64) { >> Ah, I didn't understand why we should add + 64(smb2 hdr size)... >> As I know, NextCommand offset included smb2 header size.. > > This is what I meant. > + if ((u64)work->next_smb2_rcv_hdr_off + > le32_to_cpu(hdr->NextCommand) + 64 > > + get_rfc1002_len(work->request_buf)) { > > It could just be an early check that what hdr->NextCommand points to > has at least 64 bytes. > I.e. an early test that "does the next PDU have at least a full smb2 > header?" > > I mean, since you already test that NextCommand is valid, you could > at the same time also > test that the next pdu is at least 64 bytes. Understood, I will update it on v3. Thanks! > >> > >> > >> > Which leads to another question. Where do you check that the buffer >> > contains enough data to hold the smb2 header and the full fixed part >> > of the request? >> ksmbd_smb2_check_message() in smb2misc.c should check it. >> >> > There is a check that you have enough space for the smb2 header in >> > ksmbd_conn_handler_loop() >> > that there is enough space for the smb2 header >> > (ksmbd_pdu_size_has_room()) but that function assumes that the smb2 >> > header always start at the head of the buffer. >> > So if you have a compound chain, this functrion only checks the first >> > pdu. >> I think that is_chained_smb2_message() will check all pdu as well as first >> pdu. >> there is loop do { } while (is_chained_smb2_message(work)); in server.c >> > >> > >> > I know that the buffer handling is copied from the cifs client. It >> > used to also do these "just pass a buffer around and the first 4 bytes >> > is the size" (and still does for smb1) and there was a lot of >> > terrible +4 or -4 to all sort of casts and conditionals. >> > I changed that in cifs.ko to remove the 4 byte length completely from >> > the buffer. >> > I also changed it as part of the compounding to pass an array of >> > requests (each containing an iovector) to the functions instead of >> > just one large byte array. >> > That made things a lot easier to manage since you could then assume >> > that the SMB2 header would always start at offset 0 in the >> > corresponding iovector, even for compounded commands since they all >> > had their own private vector. >> > And since an iovector contains both a pointer and a length there is no >> > need anymore to read the first 4 bytes/validate them/and covnert into >> > a length all the time. >> Right. fully agreed. >> >> > >> > I think that would help, but it would be a MAJOR amount of work, so >> > maybe that should wait until later. >> Agreed, I will do that after fixing current urgent issues first! >> >> > That approach is very nice since it completely avoids keeping track of >> > offset-to-where-this-pdu-starts which makes all checks and >> > conditionals so much more complex. >> Thanks! >> > >> > >> > regards >> > ronnie sahlberg >> > >> > >> >> -- >> >> 2.25.1 >> >> >> > >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index 1fe37ad4e5bc..cae796ea1148 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work *work) hdr = ksmbd_req_buf_next(work); if (le32_to_cpu(hdr->NextCommand) > 0) { + if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) > + get_rfc1002_len(work->request_buf)) { + pr_err("next command(%u) offset exceeds smb msg size\n", + hdr->NextCommand); + 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: 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. fs/ksmbd/smb2pdu.c | 7 +++++++ 1 file changed, 7 insertions(+)