| Message ID | 20210922022811.654412-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ksmbd: add validation in smb2_ioctl | expand |
I hope I look at the right patches/branches, appologies if not. Do you have a branch where you have all these patches applied? On Wed, Sep 22, 2021 at 12:28 PM Namjae Jeon <linkinjeon@kernel.org> wrote: > > Add validation for request/response buffer size check in smb2_ioctl and > fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments > instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req > argument of fsctl_validate_negotiate_info. > > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > v2: > - fix warning: variable 'ret' is used uninitialized ret. > v3: > - fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments > instead of smb2_ioctl_req structure. > - remove an unused smb2_ioctl_req argument of fsctl_validate_negotiate_info. > fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 68 insertions(+), 19 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index aaf50f677194..e96491c9ab92 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -6986,24 +6986,26 @@ int smb2_lock(struct ksmbd_work *work) > return err; > } > > -static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, > +static int fsctl_copychunk(struct ksmbd_work *work, > + struct copychunk_ioctl_req *ci_req, > + unsigned int cnt_code, > + unsigned int input_count, > + unsigned long long volatile_id, > + unsigned long long persistent_id, > struct smb2_ioctl_rsp *rsp) > { > - struct copychunk_ioctl_req *ci_req; > struct copychunk_ioctl_rsp *ci_rsp; > struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; > struct srv_copychunk *chunks; > unsigned int i, chunk_count, chunk_count_written = 0; > unsigned int chunk_size_written = 0; > loff_t total_size_written = 0; > - int ret, cnt_code; > + int ret = 0; > > - cnt_code = le32_to_cpu(req->CntCode); > - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; > ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; > > - rsp->VolatileFileId = req->VolatileFileId; > - rsp->PersistentFileId = req->PersistentFileId; > + rsp->VolatileFileId = cpu_to_le64(volatile_id); > + rsp->PersistentFileId = cpu_to_le64(persistent_id); > ci_rsp->ChunksWritten = > cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); > ci_rsp->ChunkBytesWritten = > @@ -7013,12 +7015,13 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, > > chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; > chunk_count = le32_to_cpu(ci_req->ChunkCount); > + if (chunk_count == 0) > + goto out; > total_size_written = 0; > > /* verify the SRV_COPYCHUNK_COPY packet */ > if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || > - le32_to_cpu(req->InputCount) < > - offsetof(struct copychunk_ioctl_req, Chunks) + > + input_count < offsetof(struct copychunk_ioctl_req, Chunks) + > chunk_count * sizeof(struct srv_copychunk)) { > rsp->hdr.Status = STATUS_INVALID_PARAMETER; > return -EINVAL; > @@ -7039,9 +7042,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, > > src_fp = ksmbd_lookup_foreign_fd(work, > le64_to_cpu(ci_req->ResumeKey[0])); > - dst_fp = ksmbd_lookup_fd_slow(work, > - le64_to_cpu(req->VolatileFileId), > - le64_to_cpu(req->PersistentFileId)); > + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id); > ret = -EINVAL; > if (!src_fp || > src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) { > @@ -7116,8 +7117,8 @@ static __be32 idev_ipv4_address(struct in_device *idev) > } > > static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, > - struct smb2_ioctl_req *req, > - struct smb2_ioctl_rsp *rsp) > + struct smb2_ioctl_rsp *rsp, > + int out_buf_len) > { > struct network_interface_info_ioctl_rsp *nii_rsp = NULL; > int nbytes = 0; > @@ -7200,6 +7201,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, > sockaddr_storage->addr6.ScopeId = 0; > } > > + if (out_buf_len < sizeof(struct network_interface_info_ioctl_rsp)) > + break; > nbytes += sizeof(struct network_interface_info_ioctl_rsp); > } > rtnl_unlock(); > @@ -7220,11 +7223,16 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, > > static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn, > struct validate_negotiate_info_req *neg_req, > - struct validate_negotiate_info_rsp *neg_rsp) > + struct validate_negotiate_info_rsp *neg_rsp, > + int in_buf_len) > { > int ret = 0; > int dialect; > > + if (in_buf_len < sizeof(struct validate_negotiate_info_req) + > + le16_to_cpu(neg_req->DialectCount) * sizeof(__le16)) > + return -EINVAL; > + > dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects, > neg_req->DialectCount); > if (dialect == BAD_PROT_ID || dialect != conn->dialect) { > @@ -7400,7 +7408,7 @@ int smb2_ioctl(struct ksmbd_work *work) > struct smb2_ioctl_req *req; > struct smb2_ioctl_rsp *rsp, *rsp_org; > int cnt_code, nbytes = 0; > - int out_buf_len; > + int out_buf_len, in_buf_len; > u64 id = KSMBD_NO_FID; > struct ksmbd_conn *conn = work->conn; > int ret = 0; > @@ -7430,6 +7438,7 @@ int smb2_ioctl(struct ksmbd_work *work) > cnt_code = le32_to_cpu(req->CntCode); > out_buf_len = le32_to_cpu(req->MaxOutputResponse); > out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len); > + in_buf_len = le32_to_cpu(req->InputCount); Do you check if you have these many bytes remaining in the buffer? Also earlier in this function, where you assign req. If this is not a component in a compound, then you assign req = work->request_buf which starts with 4 bytes for the length and the smb2_hdr starts at offset 4, right? But if the ioctl is inside a compound, you assign req = ksmbd_req_buf_next() which just returns the offset to wherever NextCommand is? There are two problems here. Now req points to something where teh smb2 header starts at offset 0 instead of 4. I unfortunately do not have any code to create Create/Ioctl/Close compounds to test this, but it looks like this is a problem. The second is that there is no check I can see that we validate that req now points to valid data, which means that when we dereference req just a few lines later (req->VolatileFileId) we might read random data beyond the end of request_buf or we might oops. The same might be an issue in other functions as well. > > switch (cnt_code) { > case FSCTL_DFS_GET_REFERRALS: > @@ -7465,9 +7474,16 @@ int smb2_ioctl(struct ksmbd_work *work) > goto out; > } > > + if (in_buf_len < sizeof(struct validate_negotiate_info_req)) > + return -EINVAL; > + > + if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) > + return -EINVAL; > + > ret = fsctl_validate_negotiate_info(conn, > (struct validate_negotiate_info_req *)&req->Buffer[0], > - (struct validate_negotiate_info_rsp *)&rsp->Buffer[0]); > + (struct validate_negotiate_info_rsp *)&rsp->Buffer[0], > + in_buf_len); > if (ret < 0) > goto out; > > @@ -7476,7 +7492,7 @@ int smb2_ioctl(struct ksmbd_work *work) > rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID); > break; > case FSCTL_QUERY_NETWORK_INTERFACE_INFO: > - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp); > + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len); > if (nbytes < 0) > goto out; > break; > @@ -7503,15 +7519,33 @@ int smb2_ioctl(struct ksmbd_work *work) > goto out; > } > > + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) { > + ret = -EINVAL; > + goto out; > + } > + > if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) { > ret = -EINVAL; > goto out; > } > > nbytes = sizeof(struct copychunk_ioctl_rsp); > - fsctl_copychunk(work, req, rsp); > + rsp->VolatileFileId = req->VolatileFileId; > + rsp->PersistentFileId = req->PersistentFileId; > + fsctl_copychunk(work, > + (struct copychunk_ioctl_req *)&req->Buffer[0], > + le32_to_cpu(req->CntCode), > + le32_to_cpu(req->InputCount), > + le64_to_cpu(req->VolatileFileId), > + le64_to_cpu(req->PersistentFileId), > + rsp); > break; > case FSCTL_SET_SPARSE: > + if (in_buf_len < sizeof(struct file_sparse)) { > + ret = -EINVAL; > + goto out; > + } > + > ret = fsctl_set_sparse(work, id, > (struct file_sparse *)&req->Buffer[0]); > if (ret < 0) > @@ -7530,6 +7564,11 @@ int smb2_ioctl(struct ksmbd_work *work) > goto out; > } > > + if (in_buf_len < sizeof(struct file_zero_data_information)) { > + ret = -EINVAL; > + goto out; > + } > + > zero_data = > (struct file_zero_data_information *)&req->Buffer[0]; > > @@ -7549,6 +7588,11 @@ int smb2_ioctl(struct ksmbd_work *work) > break; > } > case FSCTL_QUERY_ALLOCATED_RANGES: > + if (in_buf_len < sizeof(struct file_allocated_range_buffer)) { > + ret = -EINVAL; > + goto out; > + } > + > ret = fsctl_query_allocated_ranges(work, id, > (struct file_allocated_range_buffer *)&req->Buffer[0], > (struct file_allocated_range_buffer *)&rsp->Buffer[0], > @@ -7589,6 +7633,11 @@ int smb2_ioctl(struct ksmbd_work *work) > struct duplicate_extents_to_file *dup_ext; > loff_t src_off, dst_off, length, cloned; > > + if (in_buf_len < sizeof(struct duplicate_extents_to_file)) { > + ret = -EINVAL; > + goto out; > + } > + > dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0]; > > fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle, > -- > 2.25.1 >
Ignore. Looking at the wrong code. On Wed, Sep 22, 2021 at 1:48 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > I hope I look at the right patches/branches, appologies if not. > > Do you have a branch where you have all these patches applied? > > On Wed, Sep 22, 2021 at 12:28 PM Namjae Jeon <linkinjeon@kernel.org> wrote: > > > > Add validation for request/response buffer size check in smb2_ioctl and > > fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments > > instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req > > argument of fsctl_validate_negotiate_info. > > > > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > > Cc: Ralph Böhme <slow@samba.org> > > Cc: Steve French <smfrench@gmail.com> > > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > > --- > > v2: > > - fix warning: variable 'ret' is used uninitialized ret. > > v3: > > - fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments > > instead of smb2_ioctl_req structure. > > - remove an unused smb2_ioctl_req argument of fsctl_validate_negotiate_info. > > fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 68 insertions(+), 19 deletions(-) > > > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > > index aaf50f677194..e96491c9ab92 100644 > > --- a/fs/ksmbd/smb2pdu.c > > +++ b/fs/ksmbd/smb2pdu.c > > @@ -6986,24 +6986,26 @@ int smb2_lock(struct ksmbd_work *work) > > return err; > > } > > > > -static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, > > +static int fsctl_copychunk(struct ksmbd_work *work, > > + struct copychunk_ioctl_req *ci_req, > > + unsigned int cnt_code, > > + unsigned int input_count, > > + unsigned long long volatile_id, > > + unsigned long long persistent_id, > > struct smb2_ioctl_rsp *rsp) > > { > > - struct copychunk_ioctl_req *ci_req; > > struct copychunk_ioctl_rsp *ci_rsp; > > struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; > > struct srv_copychunk *chunks; > > unsigned int i, chunk_count, chunk_count_written = 0; > > unsigned int chunk_size_written = 0; > > loff_t total_size_written = 0; > > - int ret, cnt_code; > > + int ret = 0; > > > > - cnt_code = le32_to_cpu(req->CntCode); > > - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; > > ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; > > > > - rsp->VolatileFileId = req->VolatileFileId; > > - rsp->PersistentFileId = req->PersistentFileId; > > + rsp->VolatileFileId = cpu_to_le64(volatile_id); > > + rsp->PersistentFileId = cpu_to_le64(persistent_id); > > ci_rsp->ChunksWritten = > > cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); > > ci_rsp->ChunkBytesWritten = > > @@ -7013,12 +7015,13 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, > > > > chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; > > chunk_count = le32_to_cpu(ci_req->ChunkCount); > > + if (chunk_count == 0) > > + goto out; > > total_size_written = 0; > > > > /* verify the SRV_COPYCHUNK_COPY packet */ > > if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || > > - le32_to_cpu(req->InputCount) < > > - offsetof(struct copychunk_ioctl_req, Chunks) + > > + input_count < offsetof(struct copychunk_ioctl_req, Chunks) + > > chunk_count * sizeof(struct srv_copychunk)) { > > rsp->hdr.Status = STATUS_INVALID_PARAMETER; > > return -EINVAL; > > @@ -7039,9 +7042,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, > > > > src_fp = ksmbd_lookup_foreign_fd(work, > > le64_to_cpu(ci_req->ResumeKey[0])); > > - dst_fp = ksmbd_lookup_fd_slow(work, > > - le64_to_cpu(req->VolatileFileId), > > - le64_to_cpu(req->PersistentFileId)); > > + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id); > > ret = -EINVAL; > > if (!src_fp || > > src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) { > > @@ -7116,8 +7117,8 @@ static __be32 idev_ipv4_address(struct in_device *idev) > > } > > > > static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, > > - struct smb2_ioctl_req *req, > > - struct smb2_ioctl_rsp *rsp) > > + struct smb2_ioctl_rsp *rsp, > > + int out_buf_len) > > { > > struct network_interface_info_ioctl_rsp *nii_rsp = NULL; > > int nbytes = 0; > > @@ -7200,6 +7201,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, > > sockaddr_storage->addr6.ScopeId = 0; > > } > > > > + if (out_buf_len < sizeof(struct network_interface_info_ioctl_rsp)) > > + break; > > nbytes += sizeof(struct network_interface_info_ioctl_rsp); > > } > > rtnl_unlock(); > > @@ -7220,11 +7223,16 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, > > > > static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn, > > struct validate_negotiate_info_req *neg_req, > > - struct validate_negotiate_info_rsp *neg_rsp) > > + struct validate_negotiate_info_rsp *neg_rsp, > > + int in_buf_len) > > { > > int ret = 0; > > int dialect; > > > > + if (in_buf_len < sizeof(struct validate_negotiate_info_req) + > > + le16_to_cpu(neg_req->DialectCount) * sizeof(__le16)) > > + return -EINVAL; > > + > > dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects, > > neg_req->DialectCount); > > if (dialect == BAD_PROT_ID || dialect != conn->dialect) { > > @@ -7400,7 +7408,7 @@ int smb2_ioctl(struct ksmbd_work *work) > > struct smb2_ioctl_req *req; > > struct smb2_ioctl_rsp *rsp, *rsp_org; > > int cnt_code, nbytes = 0; > > - int out_buf_len; > > + int out_buf_len, in_buf_len; > > u64 id = KSMBD_NO_FID; > > struct ksmbd_conn *conn = work->conn; > > int ret = 0; > > @@ -7430,6 +7438,7 @@ int smb2_ioctl(struct ksmbd_work *work) > > cnt_code = le32_to_cpu(req->CntCode); > > out_buf_len = le32_to_cpu(req->MaxOutputResponse); > > out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len); > > + in_buf_len = le32_to_cpu(req->InputCount); > > Do you check if you have these many bytes remaining in the buffer? > > Also earlier in this function, where you assign req. > If this is not a component in a compound, then you assign req = > work->request_buf > which starts with 4 bytes for the length and the smb2_hdr starts at > offset 4, right? > > But if the ioctl is inside a compound, you assign req = ksmbd_req_buf_next() > which just returns the offset to wherever NextCommand is? > There are two problems here. > Now req points to something where teh smb2 header starts at offset 0 > instead of 4. > I unfortunately do not have any code to create Create/Ioctl/Close > compounds to test this, > but it looks like this is a problem. > > The second is that there is no check I can see that we validate that > req now points to valid data, > which means that when we dereference req just a few lines later > (req->VolatileFileId) > we might read random data beyond the end of request_buf or we might oops. > > > The same might be an issue in other functions as well. > > > > > > switch (cnt_code) { > > case FSCTL_DFS_GET_REFERRALS: > > @@ -7465,9 +7474,16 @@ int smb2_ioctl(struct ksmbd_work *work) > > goto out; > > } > > > > + if (in_buf_len < sizeof(struct validate_negotiate_info_req)) > > + return -EINVAL; > > + > > + if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) > > + return -EINVAL; > > + > > ret = fsctl_validate_negotiate_info(conn, > > (struct validate_negotiate_info_req *)&req->Buffer[0], > > - (struct validate_negotiate_info_rsp *)&rsp->Buffer[0]); > > + (struct validate_negotiate_info_rsp *)&rsp->Buffer[0], > > + in_buf_len); > > if (ret < 0) > > goto out; > > > > @@ -7476,7 +7492,7 @@ int smb2_ioctl(struct ksmbd_work *work) > > rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID); > > break; > > case FSCTL_QUERY_NETWORK_INTERFACE_INFO: > > - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp); > > + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len); > > if (nbytes < 0) > > goto out; > > break; > > @@ -7503,15 +7519,33 @@ int smb2_ioctl(struct ksmbd_work *work) > > goto out; > > } > > > > + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) { > > ret = -EINVAL; > > goto out; > > } > > > > nbytes = sizeof(struct copychunk_ioctl_rsp); > > - fsctl_copychunk(work, req, rsp); > > + rsp->VolatileFileId = req->VolatileFileId; > > + rsp->PersistentFileId = req->PersistentFileId; > > + fsctl_copychunk(work, > > + (struct copychunk_ioctl_req *)&req->Buffer[0], > > + le32_to_cpu(req->CntCode), > > + le32_to_cpu(req->InputCount), > > + le64_to_cpu(req->VolatileFileId), > > + le64_to_cpu(req->PersistentFileId), > > + rsp); > > break; > > case FSCTL_SET_SPARSE: > > + if (in_buf_len < sizeof(struct file_sparse)) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > ret = fsctl_set_sparse(work, id, > > (struct file_sparse *)&req->Buffer[0]); > > if (ret < 0) > > @@ -7530,6 +7564,11 @@ int smb2_ioctl(struct ksmbd_work *work) > > goto out; > > } > > > > + if (in_buf_len < sizeof(struct file_zero_data_information)) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > zero_data = > > (struct file_zero_data_information *)&req->Buffer[0]; > > > > @@ -7549,6 +7588,11 @@ int smb2_ioctl(struct ksmbd_work *work) > > break; > > } > > case FSCTL_QUERY_ALLOCATED_RANGES: > > + if (in_buf_len < sizeof(struct file_allocated_range_buffer)) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > ret = fsctl_query_allocated_ranges(work, id, > > (struct file_allocated_range_buffer *)&req->Buffer[0], > > (struct file_allocated_range_buffer *)&rsp->Buffer[0], > > @@ -7589,6 +7633,11 @@ int smb2_ioctl(struct ksmbd_work *work) > > struct duplicate_extents_to_file *dup_ext; > > loff_t src_off, dst_off, length, cloned; > > > > + if (in_buf_len < sizeof(struct duplicate_extents_to_file)) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0]; > > > > fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle, > > -- > > 2.25.1 > >
2021-09-22 13:08 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: > Ignore. Looking at the wrong code. To avoid confusion, I have pushed all in-flight patches to my github to review correctly. Please check this branch. https://github.com/namjaejeon/smb3-kernel/tree/ksmbd-for-next Thanks! > > On Wed, Sep 22, 2021 at 1:48 PM ronnie sahlberg > <ronniesahlberg@gmail.com> wrote: >> >> I hope I look at the right patches/branches, appologies if not. >> >> Do you have a branch where you have all these patches applied? >> >> On Wed, Sep 22, 2021 at 12:28 PM Namjae Jeon <linkinjeon@kernel.org> >> wrote: >> > >> > Add validation for request/response buffer size check in smb2_ioctl and >> > fsctl_copychunk() take copychunk_ioctl_req pointer and the other >> > arguments >> > instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req >> > argument of fsctl_validate_negotiate_info. >> > >> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> > Cc: Ralph Böhme <slow@samba.org> >> > Cc: Steve French <smfrench@gmail.com> >> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> > --- >> > v2: >> > - fix warning: variable 'ret' is used uninitialized ret. >> > v3: >> > - fsctl_copychunk() take copychunk_ioctl_req pointer and the other >> > arguments >> > instead of smb2_ioctl_req structure. >> > - remove an unused smb2_ioctl_req argument of >> > fsctl_validate_negotiate_info. >> > fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++---------- >> > 1 file changed, 68 insertions(+), 19 deletions(-) >> > >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> > index aaf50f677194..e96491c9ab92 100644 >> > --- a/fs/ksmbd/smb2pdu.c >> > +++ b/fs/ksmbd/smb2pdu.c >> > @@ -6986,24 +6986,26 @@ int smb2_lock(struct ksmbd_work *work) >> > return err; >> > } >> > >> > -static int fsctl_copychunk(struct ksmbd_work *work, struct >> > smb2_ioctl_req *req, >> > +static int fsctl_copychunk(struct ksmbd_work *work, >> > + struct copychunk_ioctl_req *ci_req, >> > + unsigned int cnt_code, >> > + unsigned int input_count, >> > + unsigned long long volatile_id, >> > + unsigned long long persistent_id, >> > struct smb2_ioctl_rsp *rsp) >> > { >> > - struct copychunk_ioctl_req *ci_req; >> > struct copychunk_ioctl_rsp *ci_rsp; >> > struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; >> > struct srv_copychunk *chunks; >> > unsigned int i, chunk_count, chunk_count_written = 0; >> > unsigned int chunk_size_written = 0; >> > loff_t total_size_written = 0; >> > - int ret, cnt_code; >> > + int ret = 0; >> > >> > - cnt_code = le32_to_cpu(req->CntCode); >> > - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; >> > ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; >> > >> > - rsp->VolatileFileId = req->VolatileFileId; >> > - rsp->PersistentFileId = req->PersistentFileId; >> > + rsp->VolatileFileId = cpu_to_le64(volatile_id); >> > + rsp->PersistentFileId = cpu_to_le64(persistent_id); >> > ci_rsp->ChunksWritten = >> > cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); >> > ci_rsp->ChunkBytesWritten = >> > @@ -7013,12 +7015,13 @@ static int fsctl_copychunk(struct ksmbd_work >> > *work, struct smb2_ioctl_req *req, >> > >> > chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; >> > chunk_count = le32_to_cpu(ci_req->ChunkCount); >> > + if (chunk_count == 0) >> > + goto out; >> > total_size_written = 0; >> > >> > /* verify the SRV_COPYCHUNK_COPY packet */ >> > if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || >> > - le32_to_cpu(req->InputCount) < >> > - offsetof(struct copychunk_ioctl_req, Chunks) + >> > + input_count < offsetof(struct copychunk_ioctl_req, Chunks) >> > + >> > chunk_count * sizeof(struct srv_copychunk)) { >> > rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> > return -EINVAL; >> > @@ -7039,9 +7042,7 @@ static int fsctl_copychunk(struct ksmbd_work >> > *work, struct smb2_ioctl_req *req, >> > >> > src_fp = ksmbd_lookup_foreign_fd(work, >> > >> > le64_to_cpu(ci_req->ResumeKey[0])); >> > - dst_fp = ksmbd_lookup_fd_slow(work, >> > - le64_to_cpu(req->VolatileFileId), >> > - >> > le64_to_cpu(req->PersistentFileId)); >> > + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, >> > persistent_id); >> > ret = -EINVAL; >> > if (!src_fp || >> > src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) >> > { >> > @@ -7116,8 +7117,8 @@ static __be32 idev_ipv4_address(struct in_device >> > *idev) >> > } >> > >> > static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, >> > - struct smb2_ioctl_req *req, >> > - struct smb2_ioctl_rsp *rsp) >> > + struct smb2_ioctl_rsp *rsp, >> > + int out_buf_len) >> > { >> > struct network_interface_info_ioctl_rsp *nii_rsp = NULL; >> > int nbytes = 0; >> > @@ -7200,6 +7201,8 @@ static int fsctl_query_iface_info_ioctl(struct >> > ksmbd_conn *conn, >> > sockaddr_storage->addr6.ScopeId = 0; >> > } >> > >> > + if (out_buf_len < sizeof(struct >> > network_interface_info_ioctl_rsp)) >> > + break; >> > nbytes += sizeof(struct >> > network_interface_info_ioctl_rsp); >> > } >> > rtnl_unlock(); >> > @@ -7220,11 +7223,16 @@ static int fsctl_query_iface_info_ioctl(struct >> > ksmbd_conn *conn, >> > >> > static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn, >> > struct >> > validate_negotiate_info_req *neg_req, >> > - struct >> > validate_negotiate_info_rsp *neg_rsp) >> > + struct >> > validate_negotiate_info_rsp *neg_rsp, >> > + int in_buf_len) >> > { >> > int ret = 0; >> > int dialect; >> > >> > + if (in_buf_len < sizeof(struct validate_negotiate_info_req) + >> > + le16_to_cpu(neg_req->DialectCount) * >> > sizeof(__le16)) >> > + return -EINVAL; >> > + >> > dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects, >> > neg_req->DialectCount); >> > if (dialect == BAD_PROT_ID || dialect != conn->dialect) { >> > @@ -7400,7 +7408,7 @@ int smb2_ioctl(struct ksmbd_work *work) >> > struct smb2_ioctl_req *req; >> > struct smb2_ioctl_rsp *rsp, *rsp_org; >> > int cnt_code, nbytes = 0; >> > - int out_buf_len; >> > + int out_buf_len, in_buf_len; >> > u64 id = KSMBD_NO_FID; >> > struct ksmbd_conn *conn = work->conn; >> > int ret = 0; >> > @@ -7430,6 +7438,7 @@ int smb2_ioctl(struct ksmbd_work *work) >> > cnt_code = le32_to_cpu(req->CntCode); >> > out_buf_len = le32_to_cpu(req->MaxOutputResponse); >> > out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len); >> > + in_buf_len = le32_to_cpu(req->InputCount); >> >> Do you check if you have these many bytes remaining in the buffer? >> >> Also earlier in this function, where you assign req. >> If this is not a component in a compound, then you assign req = >> work->request_buf >> which starts with 4 bytes for the length and the smb2_hdr starts at >> offset 4, right? >> >> But if the ioctl is inside a compound, you assign req = >> ksmbd_req_buf_next() >> which just returns the offset to wherever NextCommand is? >> There are two problems here. >> Now req points to something where teh smb2 header starts at offset 0 >> instead of 4. >> I unfortunately do not have any code to create Create/Ioctl/Close >> compounds to test this, >> but it looks like this is a problem. >> >> The second is that there is no check I can see that we validate that >> req now points to valid data, >> which means that when we dereference req just a few lines later >> (req->VolatileFileId) >> we might read random data beyond the end of request_buf or we might >> oops. >> >> >> The same might be an issue in other functions as well. >> >> >> > >> > switch (cnt_code) { >> > case FSCTL_DFS_GET_REFERRALS: >> > @@ -7465,9 +7474,16 @@ int smb2_ioctl(struct ksmbd_work *work) >> > goto out; >> > } >> > >> > + if (in_buf_len < sizeof(struct >> > validate_negotiate_info_req)) >> > + return -EINVAL; >> > + >> > + if (out_buf_len < sizeof(struct >> > validate_negotiate_info_rsp)) >> > + return -EINVAL; >> > + >> > ret = fsctl_validate_negotiate_info(conn, >> > (struct validate_negotiate_info_req >> > *)&req->Buffer[0], >> > - (struct validate_negotiate_info_rsp >> > *)&rsp->Buffer[0]); >> > + (struct validate_negotiate_info_rsp >> > *)&rsp->Buffer[0], >> > + in_buf_len); >> > if (ret < 0) >> > goto out; >> > >> > @@ -7476,7 +7492,7 @@ int smb2_ioctl(struct ksmbd_work *work) >> > rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID); >> > break; >> > case FSCTL_QUERY_NETWORK_INTERFACE_INFO: >> > - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp); >> > + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, >> > out_buf_len); >> > if (nbytes < 0) >> > goto out; >> > break; >> > @@ -7503,15 +7519,33 @@ int smb2_ioctl(struct ksmbd_work *work) >> > goto out; >> > } >> > >> > + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) { >> > + ret = -EINVAL; >> > + goto out; >> > + } >> > + >> > if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) { >> > ret = -EINVAL; >> > goto out; >> > } >> > >> > nbytes = sizeof(struct copychunk_ioctl_rsp); >> > - fsctl_copychunk(work, req, rsp); >> > + rsp->VolatileFileId = req->VolatileFileId; >> > + rsp->PersistentFileId = req->PersistentFileId; >> > + fsctl_copychunk(work, >> > + (struct copychunk_ioctl_req >> > *)&req->Buffer[0], >> > + le32_to_cpu(req->CntCode), >> > + le32_to_cpu(req->InputCount), >> > + le64_to_cpu(req->VolatileFileId), >> > + le64_to_cpu(req->PersistentFileId), >> > + rsp); >> > break; >> > case FSCTL_SET_SPARSE: >> > + if (in_buf_len < sizeof(struct file_sparse)) { >> > + ret = -EINVAL; >> > + goto out; >> > + } >> > + >> > ret = fsctl_set_sparse(work, id, >> > (struct file_sparse >> > *)&req->Buffer[0]); >> > if (ret < 0) >> > @@ -7530,6 +7564,11 @@ int smb2_ioctl(struct ksmbd_work *work) >> > goto out; >> > } >> > >> > + if (in_buf_len < sizeof(struct >> > file_zero_data_information)) { >> > + ret = -EINVAL; >> > + goto out; >> > + } >> > + >> > zero_data = >> > (struct file_zero_data_information >> > *)&req->Buffer[0]; >> > >> > @@ -7549,6 +7588,11 @@ int smb2_ioctl(struct ksmbd_work *work) >> > break; >> > } >> > case FSCTL_QUERY_ALLOCATED_RANGES: >> > + if (in_buf_len < sizeof(struct >> > file_allocated_range_buffer)) { >> > + ret = -EINVAL; >> > + goto out; >> > + } >> > + >> > ret = fsctl_query_allocated_ranges(work, id, >> > (struct file_allocated_range_buffer >> > *)&req->Buffer[0], >> > (struct file_allocated_range_buffer >> > *)&rsp->Buffer[0], >> > @@ -7589,6 +7633,11 @@ int smb2_ioctl(struct ksmbd_work *work) >> > struct duplicate_extents_to_file *dup_ext; >> > loff_t src_off, dst_off, length, cloned; >> > >> > + if (in_buf_len < sizeof(struct >> > duplicate_extents_to_file)) { >> > + ret = -EINVAL; >> > + goto out; >> > + } >> > + >> > dup_ext = (struct duplicate_extents_to_file >> > *)&req->Buffer[0]; >> > >> > fp_in = ksmbd_lookup_fd_slow(work, >> > dup_ext->VolatileFileHandle, >> > -- >> > 2.25.1 >> > >
On 9/22/2021 12:13 AM, Namjae Jeon wrote: > 2021-09-22 13:08 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: >> Ignore. Looking at the wrong code. > To avoid confusion, I have pushed all in-flight patches to my github > to review correctly. > Please check this branch. > https://github.com/namjaejeon/smb3-kernel/tree/ksmbd-for-next I don't understand where to look for patches. You continue to send the git diffs to email, and that's the generally preferred method for review. I have comments on the code below. Do I have to go review on github? Tom. > > Thanks! > >> >> On Wed, Sep 22, 2021 at 1:48 PM ronnie sahlberg >> <ronniesahlberg@gmail.com> wrote: >>> >>> I hope I look at the right patches/branches, appologies if not. >>> >>> Do you have a branch where you have all these patches applied? >>> >>> On Wed, Sep 22, 2021 at 12:28 PM Namjae Jeon <linkinjeon@kernel.org> >>> wrote: >>>> >>>> Add validation for request/response buffer size check in smb2_ioctl and >>>> fsctl_copychunk() take copychunk_ioctl_req pointer and the other >>>> arguments >>>> instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req >>>> argument of fsctl_validate_negotiate_info. >>>> >>>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >>>> Cc: Ralph Böhme <slow@samba.org> >>>> Cc: Steve French <smfrench@gmail.com> >>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>>> --- >>>> v2: >>>> - fix warning: variable 'ret' is used uninitialized ret. >>>> v3: >>>> - fsctl_copychunk() take copychunk_ioctl_req pointer and the other >>>> arguments >>>> instead of smb2_ioctl_req structure. >>>> - remove an unused smb2_ioctl_req argument of >>>> fsctl_validate_negotiate_info. >>>> fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++---------- >>>> 1 file changed, 68 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >>>> index aaf50f677194..e96491c9ab92 100644 >>>> --- a/fs/ksmbd/smb2pdu.c >>>> +++ b/fs/ksmbd/smb2pdu.c >>>> @@ -6986,24 +6986,26 @@ int smb2_lock(struct ksmbd_work *work) >>>> return err; >>>> } >>>> >>>> -static int fsctl_copychunk(struct ksmbd_work *work, struct >>>> smb2_ioctl_req *req, >>>> +static int fsctl_copychunk(struct ksmbd_work *work, >>>> + struct copychunk_ioctl_req *ci_req, >>>> + unsigned int cnt_code, >>>> + unsigned int input_count, >>>> + unsigned long long volatile_id, >>>> + unsigned long long persistent_id, >>>> struct smb2_ioctl_rsp *rsp) >>>> { >>>> - struct copychunk_ioctl_req *ci_req; >>>> struct copychunk_ioctl_rsp *ci_rsp; >>>> struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; >>>> struct srv_copychunk *chunks; >>>> unsigned int i, chunk_count, chunk_count_written = 0; >>>> unsigned int chunk_size_written = 0; >>>> loff_t total_size_written = 0; >>>> - int ret, cnt_code; >>>> + int ret = 0; >>>> >>>> - cnt_code = le32_to_cpu(req->CntCode); >>>> - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; >>>> ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; >>>> >>>> - rsp->VolatileFileId = req->VolatileFileId; >>>> - rsp->PersistentFileId = req->PersistentFileId; >>>> + rsp->VolatileFileId = cpu_to_le64(volatile_id); >>>> + rsp->PersistentFileId = cpu_to_le64(persistent_id); >>>> ci_rsp->ChunksWritten = >>>> cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); >>>> ci_rsp->ChunkBytesWritten = >>>> @@ -7013,12 +7015,13 @@ static int fsctl_copychunk(struct ksmbd_work >>>> *work, struct smb2_ioctl_req *req, >>>> >>>> chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; >>>> chunk_count = le32_to_cpu(ci_req->ChunkCount); >>>> + if (chunk_count == 0) >>>> + goto out; >>>> total_size_written = 0; >>>> >>>> /* verify the SRV_COPYCHUNK_COPY packet */ >>>> if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || >>>> - le32_to_cpu(req->InputCount) < >>>> - offsetof(struct copychunk_ioctl_req, Chunks) + >>>> + input_count < offsetof(struct copychunk_ioctl_req, Chunks) >>>> + >>>> chunk_count * sizeof(struct srv_copychunk)) { >>>> rsp->hdr.Status = STATUS_INVALID_PARAMETER; >>>> return -EINVAL; >>>> @@ -7039,9 +7042,7 @@ static int fsctl_copychunk(struct ksmbd_work >>>> *work, struct smb2_ioctl_req *req, >>>> >>>> src_fp = ksmbd_lookup_foreign_fd(work, >>>> >>>> le64_to_cpu(ci_req->ResumeKey[0])); >>>> - dst_fp = ksmbd_lookup_fd_slow(work, >>>> - le64_to_cpu(req->VolatileFileId), >>>> - >>>> le64_to_cpu(req->PersistentFileId)); >>>> + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, >>>> persistent_id); >>>> ret = -EINVAL; >>>> if (!src_fp || >>>> src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) >>>> { >>>> @@ -7116,8 +7117,8 @@ static __be32 idev_ipv4_address(struct in_device >>>> *idev) >>>> } >>>> >>>> static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, >>>> - struct smb2_ioctl_req *req, >>>> - struct smb2_ioctl_rsp *rsp) >>>> + struct smb2_ioctl_rsp *rsp, >>>> + int out_buf_len) >>>> { >>>> struct network_interface_info_ioctl_rsp *nii_rsp = NULL; >>>> int nbytes = 0; >>>> @@ -7200,6 +7201,8 @@ static int fsctl_query_iface_info_ioctl(struct >>>> ksmbd_conn *conn, >>>> sockaddr_storage->addr6.ScopeId = 0; >>>> } >>>> >>>> + if (out_buf_len < sizeof(struct >>>> network_interface_info_ioctl_rsp)) >>>> + break; >>>> nbytes += sizeof(struct >>>> network_interface_info_ioctl_rsp); >>>> } >>>> rtnl_unlock(); >>>> @@ -7220,11 +7223,16 @@ static int fsctl_query_iface_info_ioctl(struct >>>> ksmbd_conn *conn, >>>> >>>> static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn, >>>> struct >>>> validate_negotiate_info_req *neg_req, >>>> - struct >>>> validate_negotiate_info_rsp *neg_rsp) >>>> + struct >>>> validate_negotiate_info_rsp *neg_rsp, >>>> + int in_buf_len) >>>> { >>>> int ret = 0; >>>> int dialect; >>>> >>>> + if (in_buf_len < sizeof(struct validate_negotiate_info_req) + >>>> + le16_to_cpu(neg_req->DialectCount) * >>>> sizeof(__le16)) >>>> + return -EINVAL; >>>> + >>>> dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects, >>>> neg_req->DialectCount); >>>> if (dialect == BAD_PROT_ID || dialect != conn->dialect) { >>>> @@ -7400,7 +7408,7 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> struct smb2_ioctl_req *req; >>>> struct smb2_ioctl_rsp *rsp, *rsp_org; >>>> int cnt_code, nbytes = 0; >>>> - int out_buf_len; >>>> + int out_buf_len, in_buf_len; >>>> u64 id = KSMBD_NO_FID; >>>> struct ksmbd_conn *conn = work->conn; >>>> int ret = 0; >>>> @@ -7430,6 +7438,7 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> cnt_code = le32_to_cpu(req->CntCode); >>>> out_buf_len = le32_to_cpu(req->MaxOutputResponse); >>>> out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len); >>>> + in_buf_len = le32_to_cpu(req->InputCount); >>> >>> Do you check if you have these many bytes remaining in the buffer? >>> >>> Also earlier in this function, where you assign req. >>> If this is not a component in a compound, then you assign req = >>> work->request_buf >>> which starts with 4 bytes for the length and the smb2_hdr starts at >>> offset 4, right? >>> >>> But if the ioctl is inside a compound, you assign req = >>> ksmbd_req_buf_next() >>> which just returns the offset to wherever NextCommand is? >>> There are two problems here. >>> Now req points to something where teh smb2 header starts at offset 0 >>> instead of 4. >>> I unfortunately do not have any code to create Create/Ioctl/Close >>> compounds to test this, >>> but it looks like this is a problem. >>> >>> The second is that there is no check I can see that we validate that >>> req now points to valid data, >>> which means that when we dereference req just a few lines later >>> (req->VolatileFileId) >>> we might read random data beyond the end of request_buf or we might >>> oops. >>> >>> >>> The same might be an issue in other functions as well. >>> >>> >>>> >>>> switch (cnt_code) { >>>> case FSCTL_DFS_GET_REFERRALS: >>>> @@ -7465,9 +7474,16 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> goto out; >>>> } >>>> >>>> + if (in_buf_len < sizeof(struct >>>> validate_negotiate_info_req)) >>>> + return -EINVAL; >>>> + >>>> + if (out_buf_len < sizeof(struct >>>> validate_negotiate_info_rsp)) >>>> + return -EINVAL; >>>> + >>>> ret = fsctl_validate_negotiate_info(conn, >>>> (struct validate_negotiate_info_req >>>> *)&req->Buffer[0], >>>> - (struct validate_negotiate_info_rsp >>>> *)&rsp->Buffer[0]); >>>> + (struct validate_negotiate_info_rsp >>>> *)&rsp->Buffer[0], >>>> + in_buf_len); >>>> if (ret < 0) >>>> goto out; >>>> >>>> @@ -7476,7 +7492,7 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID); >>>> break; >>>> case FSCTL_QUERY_NETWORK_INTERFACE_INFO: >>>> - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp); >>>> + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, >>>> out_buf_len); >>>> if (nbytes < 0) >>>> goto out; >>>> break; >>>> @@ -7503,15 +7519,33 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> goto out; >>>> } >>>> >>>> + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) { >>>> + ret = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) { >>>> ret = -EINVAL; >>>> goto out; >>>> } >>>> >>>> nbytes = sizeof(struct copychunk_ioctl_rsp); >>>> - fsctl_copychunk(work, req, rsp); >>>> + rsp->VolatileFileId = req->VolatileFileId; >>>> + rsp->PersistentFileId = req->PersistentFileId; >>>> + fsctl_copychunk(work, >>>> + (struct copychunk_ioctl_req >>>> *)&req->Buffer[0], >>>> + le32_to_cpu(req->CntCode), >>>> + le32_to_cpu(req->InputCount), >>>> + le64_to_cpu(req->VolatileFileId), >>>> + le64_to_cpu(req->PersistentFileId), >>>> + rsp); >>>> break; >>>> case FSCTL_SET_SPARSE: >>>> + if (in_buf_len < sizeof(struct file_sparse)) { >>>> + ret = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> ret = fsctl_set_sparse(work, id, >>>> (struct file_sparse >>>> *)&req->Buffer[0]); >>>> if (ret < 0) >>>> @@ -7530,6 +7564,11 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> goto out; >>>> } >>>> >>>> + if (in_buf_len < sizeof(struct >>>> file_zero_data_information)) { >>>> + ret = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> zero_data = >>>> (struct file_zero_data_information >>>> *)&req->Buffer[0]; >>>> >>>> @@ -7549,6 +7588,11 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> break; >>>> } >>>> case FSCTL_QUERY_ALLOCATED_RANGES: >>>> + if (in_buf_len < sizeof(struct >>>> file_allocated_range_buffer)) { >>>> + ret = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> ret = fsctl_query_allocated_ranges(work, id, >>>> (struct file_allocated_range_buffer >>>> *)&req->Buffer[0], >>>> (struct file_allocated_range_buffer >>>> *)&rsp->Buffer[0], >>>> @@ -7589,6 +7633,11 @@ int smb2_ioctl(struct ksmbd_work *work) >>>> struct duplicate_extents_to_file *dup_ext; >>>> loff_t src_off, dst_off, length, cloned; >>>> >>>> + if (in_buf_len < sizeof(struct >>>> duplicate_extents_to_file)) { >>>> + ret = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> dup_ext = (struct duplicate_extents_to_file >>>> *)&req->Buffer[0]; >>>> >>>> fp_in = ksmbd_lookup_fd_slow(work, >>>> dup_ext->VolatileFileHandle, >>>> -- >>>> 2.25.1 >>>> >> >
2021-09-24 0:02 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/22/2021 12:13 AM, Namjae Jeon wrote: >> 2021-09-22 13:08 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: >>> Ignore. Looking at the wrong code. >> To avoid confusion, I have pushed all in-flight patches to my github >> to review correctly. >> Please check this branch. >> https://github.com/namjaejeon/smb3-kernel/tree/ksmbd-for-next > > I don't understand where to look for patches. You continue to send > the git diffs to email, and that's the generally preferred method > for review. > > I have comments on the code below. Do I have to go review on github? I have sent v3 patch to the list before, If you don't look for it, I will re-send the patch to list. > > Tom. > > >> >> Thanks! >> >>> >>> On Wed, Sep 22, 2021 at 1:48 PM ronnie sahlberg >>> <ronniesahlberg@gmail.com> wrote: >>>> >>>> I hope I look at the right patches/branches, appologies if not. >>>> >>>> Do you have a branch where you have all these patches applied? >>>> >>>> On Wed, Sep 22, 2021 at 12:28 PM Namjae Jeon <linkinjeon@kernel.org> >>>> wrote: >>>>> >>>>> Add validation for request/response buffer size check in smb2_ioctl >>>>> and >>>>> fsctl_copychunk() take copychunk_ioctl_req pointer and the other >>>>> arguments >>>>> instead of smb2_ioctl_req structure and remove an unused >>>>> smb2_ioctl_req >>>>> argument of fsctl_validate_negotiate_info. >>>>> >>>>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >>>>> Cc: Ralph Böhme <slow@samba.org> >>>>> Cc: Steve French <smfrench@gmail.com> >>>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>>>> --- >>>>> v2: >>>>> - fix warning: variable 'ret' is used uninitialized ret. >>>>> v3: >>>>> - fsctl_copychunk() take copychunk_ioctl_req pointer and the other >>>>> arguments >>>>> instead of smb2_ioctl_req structure. >>>>> - remove an unused smb2_ioctl_req argument of >>>>> fsctl_validate_negotiate_info. >>>>> fs/ksmbd/smb2pdu.c | 87 >>>>> ++++++++++++++++++++++++++++++++++++---------- >>>>> 1 file changed, 68 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >>>>> index aaf50f677194..e96491c9ab92 100644 >>>>> --- a/fs/ksmbd/smb2pdu.c >>>>> +++ b/fs/ksmbd/smb2pdu.c >>>>> @@ -6986,24 +6986,26 @@ int smb2_lock(struct ksmbd_work *work) >>>>> return err; >>>>> } >>>>> >>>>> -static int fsctl_copychunk(struct ksmbd_work *work, struct >>>>> smb2_ioctl_req *req, >>>>> +static int fsctl_copychunk(struct ksmbd_work *work, >>>>> + struct copychunk_ioctl_req *ci_req, >>>>> + unsigned int cnt_code, >>>>> + unsigned int input_count, >>>>> + unsigned long long volatile_id, >>>>> + unsigned long long persistent_id, >>>>> struct smb2_ioctl_rsp *rsp) >>>>> { >>>>> - struct copychunk_ioctl_req *ci_req; >>>>> struct copychunk_ioctl_rsp *ci_rsp; >>>>> struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; >>>>> struct srv_copychunk *chunks; >>>>> unsigned int i, chunk_count, chunk_count_written = 0; >>>>> unsigned int chunk_size_written = 0; >>>>> loff_t total_size_written = 0; >>>>> - int ret, cnt_code; >>>>> + int ret = 0; >>>>> >>>>> - cnt_code = le32_to_cpu(req->CntCode); >>>>> - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; >>>>> ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; >>>>> >>>>> - rsp->VolatileFileId = req->VolatileFileId; >>>>> - rsp->PersistentFileId = req->PersistentFileId; >>>>> + rsp->VolatileFileId = cpu_to_le64(volatile_id); >>>>> + rsp->PersistentFileId = cpu_to_le64(persistent_id); >>>>> ci_rsp->ChunksWritten = >>>>> >>>>> cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); >>>>> ci_rsp->ChunkBytesWritten = >>>>> @@ -7013,12 +7015,13 @@ static int fsctl_copychunk(struct ksmbd_work >>>>> *work, struct smb2_ioctl_req *req, >>>>> >>>>> chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; >>>>> chunk_count = le32_to_cpu(ci_req->ChunkCount); >>>>> + if (chunk_count == 0) >>>>> + goto out; >>>>> total_size_written = 0; >>>>> >>>>> /* verify the SRV_COPYCHUNK_COPY packet */ >>>>> if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || >>>>> - le32_to_cpu(req->InputCount) < >>>>> - offsetof(struct copychunk_ioctl_req, Chunks) + >>>>> + input_count < offsetof(struct copychunk_ioctl_req, Chunks) >>>>> + >>>>> chunk_count * sizeof(struct srv_copychunk)) { >>>>> rsp->hdr.Status = STATUS_INVALID_PARAMETER; >>>>> return -EINVAL; >>>>> @@ -7039,9 +7042,7 @@ static int fsctl_copychunk(struct ksmbd_work >>>>> *work, struct smb2_ioctl_req *req, >>>>> >>>>> src_fp = ksmbd_lookup_foreign_fd(work, >>>>> >>>>> le64_to_cpu(ci_req->ResumeKey[0])); >>>>> - dst_fp = ksmbd_lookup_fd_slow(work, >>>>> - >>>>> le64_to_cpu(req->VolatileFileId), >>>>> - >>>>> le64_to_cpu(req->PersistentFileId)); >>>>> + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, >>>>> persistent_id); >>>>> ret = -EINVAL; >>>>> if (!src_fp || >>>>> src_fp->persistent_id != >>>>> le64_to_cpu(ci_req->ResumeKey[1])) >>>>> { >>>>> @@ -7116,8 +7117,8 @@ static __be32 idev_ipv4_address(struct in_device >>>>> *idev) >>>>> } >>>>> >>>>> static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, >>>>> - struct smb2_ioctl_req *req, >>>>> - struct smb2_ioctl_rsp *rsp) >>>>> + struct smb2_ioctl_rsp *rsp, >>>>> + int out_buf_len) >>>>> { >>>>> struct network_interface_info_ioctl_rsp *nii_rsp = NULL; >>>>> int nbytes = 0; >>>>> @@ -7200,6 +7201,8 @@ static int fsctl_query_iface_info_ioctl(struct >>>>> ksmbd_conn *conn, >>>>> sockaddr_storage->addr6.ScopeId = 0; >>>>> } >>>>> >>>>> + if (out_buf_len < sizeof(struct >>>>> network_interface_info_ioctl_rsp)) >>>>> + break; >>>>> nbytes += sizeof(struct >>>>> network_interface_info_ioctl_rsp); >>>>> } >>>>> rtnl_unlock(); >>>>> @@ -7220,11 +7223,16 @@ static int fsctl_query_iface_info_ioctl(struct >>>>> ksmbd_conn *conn, >>>>> >>>>> static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn, >>>>> struct >>>>> validate_negotiate_info_req *neg_req, >>>>> - struct >>>>> validate_negotiate_info_rsp *neg_rsp) >>>>> + struct >>>>> validate_negotiate_info_rsp *neg_rsp, >>>>> + int in_buf_len) >>>>> { >>>>> int ret = 0; >>>>> int dialect; >>>>> >>>>> + if (in_buf_len < sizeof(struct validate_negotiate_info_req) + >>>>> + le16_to_cpu(neg_req->DialectCount) * >>>>> sizeof(__le16)) >>>>> + return -EINVAL; >>>>> + >>>>> dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects, >>>>> neg_req->DialectCount); >>>>> if (dialect == BAD_PROT_ID || dialect != conn->dialect) { >>>>> @@ -7400,7 +7408,7 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> struct smb2_ioctl_req *req; >>>>> struct smb2_ioctl_rsp *rsp, *rsp_org; >>>>> int cnt_code, nbytes = 0; >>>>> - int out_buf_len; >>>>> + int out_buf_len, in_buf_len; >>>>> u64 id = KSMBD_NO_FID; >>>>> struct ksmbd_conn *conn = work->conn; >>>>> int ret = 0; >>>>> @@ -7430,6 +7438,7 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> cnt_code = le32_to_cpu(req->CntCode); >>>>> out_buf_len = le32_to_cpu(req->MaxOutputResponse); >>>>> out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len); >>>>> + in_buf_len = le32_to_cpu(req->InputCount); >>>> >>>> Do you check if you have these many bytes remaining in the buffer? >>>> >>>> Also earlier in this function, where you assign req. >>>> If this is not a component in a compound, then you assign req = >>>> work->request_buf >>>> which starts with 4 bytes for the length and the smb2_hdr starts at >>>> offset 4, right? >>>> >>>> But if the ioctl is inside a compound, you assign req = >>>> ksmbd_req_buf_next() >>>> which just returns the offset to wherever NextCommand is? >>>> There are two problems here. >>>> Now req points to something where teh smb2 header starts at offset 0 >>>> instead of 4. >>>> I unfortunately do not have any code to create Create/Ioctl/Close >>>> compounds to test this, >>>> but it looks like this is a problem. >>>> >>>> The second is that there is no check I can see that we validate that >>>> req now points to valid data, >>>> which means that when we dereference req just a few lines later >>>> (req->VolatileFileId) >>>> we might read random data beyond the end of request_buf or we might >>>> oops. >>>> >>>> >>>> The same might be an issue in other functions as well. >>>> >>>> >>>>> >>>>> switch (cnt_code) { >>>>> case FSCTL_DFS_GET_REFERRALS: >>>>> @@ -7465,9 +7474,16 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> goto out; >>>>> } >>>>> >>>>> + if (in_buf_len < sizeof(struct >>>>> validate_negotiate_info_req)) >>>>> + return -EINVAL; >>>>> + >>>>> + if (out_buf_len < sizeof(struct >>>>> validate_negotiate_info_rsp)) >>>>> + return -EINVAL; >>>>> + >>>>> ret = fsctl_validate_negotiate_info(conn, >>>>> (struct validate_negotiate_info_req >>>>> *)&req->Buffer[0], >>>>> - (struct validate_negotiate_info_rsp >>>>> *)&rsp->Buffer[0]); >>>>> + (struct validate_negotiate_info_rsp >>>>> *)&rsp->Buffer[0], >>>>> + in_buf_len); >>>>> if (ret < 0) >>>>> goto out; >>>>> >>>>> @@ -7476,7 +7492,7 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID); >>>>> break; >>>>> case FSCTL_QUERY_NETWORK_INTERFACE_INFO: >>>>> - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp); >>>>> + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, >>>>> out_buf_len); >>>>> if (nbytes < 0) >>>>> goto out; >>>>> break; >>>>> @@ -7503,15 +7519,33 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> goto out; >>>>> } >>>>> >>>>> + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) { >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>>> + >>>>> if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) >>>>> { >>>>> ret = -EINVAL; >>>>> goto out; >>>>> } >>>>> >>>>> nbytes = sizeof(struct copychunk_ioctl_rsp); >>>>> - fsctl_copychunk(work, req, rsp); >>>>> + rsp->VolatileFileId = req->VolatileFileId; >>>>> + rsp->PersistentFileId = req->PersistentFileId; >>>>> + fsctl_copychunk(work, >>>>> + (struct copychunk_ioctl_req >>>>> *)&req->Buffer[0], >>>>> + le32_to_cpu(req->CntCode), >>>>> + le32_to_cpu(req->InputCount), >>>>> + le64_to_cpu(req->VolatileFileId), >>>>> + le64_to_cpu(req->PersistentFileId), >>>>> + rsp); >>>>> break; >>>>> case FSCTL_SET_SPARSE: >>>>> + if (in_buf_len < sizeof(struct file_sparse)) { >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>>> + >>>>> ret = fsctl_set_sparse(work, id, >>>>> (struct file_sparse >>>>> *)&req->Buffer[0]); >>>>> if (ret < 0) >>>>> @@ -7530,6 +7564,11 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> goto out; >>>>> } >>>>> >>>>> + if (in_buf_len < sizeof(struct >>>>> file_zero_data_information)) { >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>>> + >>>>> zero_data = >>>>> (struct file_zero_data_information >>>>> *)&req->Buffer[0]; >>>>> >>>>> @@ -7549,6 +7588,11 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> break; >>>>> } >>>>> case FSCTL_QUERY_ALLOCATED_RANGES: >>>>> + if (in_buf_len < sizeof(struct >>>>> file_allocated_range_buffer)) { >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>>> + >>>>> ret = fsctl_query_allocated_ranges(work, id, >>>>> (struct file_allocated_range_buffer >>>>> *)&req->Buffer[0], >>>>> (struct file_allocated_range_buffer >>>>> *)&rsp->Buffer[0], >>>>> @@ -7589,6 +7633,11 @@ int smb2_ioctl(struct ksmbd_work *work) >>>>> struct duplicate_extents_to_file *dup_ext; >>>>> loff_t src_off, dst_off, length, cloned; >>>>> >>>>> + if (in_buf_len < sizeof(struct >>>>> duplicate_extents_to_file)) { >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>>> + >>>>> dup_ext = (struct duplicate_extents_to_file >>>>> *)&req->Buffer[0]; >>>>> >>>>> fp_in = ksmbd_lookup_fd_slow(work, >>>>> dup_ext->VolatileFileHandle, >>>>> -- >>>>> 2.25.1 >>>>> >>> >> >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index aaf50f677194..e96491c9ab92 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -6986,24 +6986,26 @@ int smb2_lock(struct ksmbd_work *work) return err; } -static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, +static int fsctl_copychunk(struct ksmbd_work *work, + struct copychunk_ioctl_req *ci_req, + unsigned int cnt_code, + unsigned int input_count, + unsigned long long volatile_id, + unsigned long long persistent_id, struct smb2_ioctl_rsp *rsp) { - struct copychunk_ioctl_req *ci_req; struct copychunk_ioctl_rsp *ci_rsp; struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; struct srv_copychunk *chunks; unsigned int i, chunk_count, chunk_count_written = 0; unsigned int chunk_size_written = 0; loff_t total_size_written = 0; - int ret, cnt_code; + int ret = 0; - cnt_code = le32_to_cpu(req->CntCode); - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; - rsp->VolatileFileId = req->VolatileFileId; - rsp->PersistentFileId = req->PersistentFileId; + rsp->VolatileFileId = cpu_to_le64(volatile_id); + rsp->PersistentFileId = cpu_to_le64(persistent_id); ci_rsp->ChunksWritten = cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); ci_rsp->ChunkBytesWritten = @@ -7013,12 +7015,13 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; chunk_count = le32_to_cpu(ci_req->ChunkCount); + if (chunk_count == 0) + goto out; total_size_written = 0; /* verify the SRV_COPYCHUNK_COPY packet */ if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || - le32_to_cpu(req->InputCount) < - offsetof(struct copychunk_ioctl_req, Chunks) + + input_count < offsetof(struct copychunk_ioctl_req, Chunks) + chunk_count * sizeof(struct srv_copychunk)) { rsp->hdr.Status = STATUS_INVALID_PARAMETER; return -EINVAL; @@ -7039,9 +7042,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req, src_fp = ksmbd_lookup_foreign_fd(work, le64_to_cpu(ci_req->ResumeKey[0])); - dst_fp = ksmbd_lookup_fd_slow(work, - le64_to_cpu(req->VolatileFileId), - le64_to_cpu(req->PersistentFileId)); + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id); ret = -EINVAL; if (!src_fp || src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) { @@ -7116,8 +7117,8 @@ static __be32 idev_ipv4_address(struct in_device *idev) } static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, - struct smb2_ioctl_req *req, - struct smb2_ioctl_rsp *rsp) + struct smb2_ioctl_rsp *rsp, + int out_buf_len) { struct network_interface_info_ioctl_rsp *nii_rsp = NULL; int nbytes = 0; @@ -7200,6 +7201,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, sockaddr_storage->addr6.ScopeId = 0; } + if (out_buf_len < sizeof(struct network_interface_info_ioctl_rsp)) + break; nbytes += sizeof(struct network_interface_info_ioctl_rsp); } rtnl_unlock(); @@ -7220,11 +7223,16 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn, static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn, struct validate_negotiate_info_req *neg_req, - struct validate_negotiate_info_rsp *neg_rsp) + struct validate_negotiate_info_rsp *neg_rsp, + int in_buf_len) { int ret = 0; int dialect; + if (in_buf_len < sizeof(struct validate_negotiate_info_req) + + le16_to_cpu(neg_req->DialectCount) * sizeof(__le16)) + return -EINVAL; + dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects, neg_req->DialectCount); if (dialect == BAD_PROT_ID || dialect != conn->dialect) { @@ -7400,7 +7408,7 @@ int smb2_ioctl(struct ksmbd_work *work) struct smb2_ioctl_req *req; struct smb2_ioctl_rsp *rsp, *rsp_org; int cnt_code, nbytes = 0; - int out_buf_len; + int out_buf_len, in_buf_len; u64 id = KSMBD_NO_FID; struct ksmbd_conn *conn = work->conn; int ret = 0; @@ -7430,6 +7438,7 @@ int smb2_ioctl(struct ksmbd_work *work) cnt_code = le32_to_cpu(req->CntCode); out_buf_len = le32_to_cpu(req->MaxOutputResponse); out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len); + in_buf_len = le32_to_cpu(req->InputCount); switch (cnt_code) { case FSCTL_DFS_GET_REFERRALS: @@ -7465,9 +7474,16 @@ int smb2_ioctl(struct ksmbd_work *work) goto out; } + if (in_buf_len < sizeof(struct validate_negotiate_info_req)) + return -EINVAL; + + if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) + return -EINVAL; + ret = fsctl_validate_negotiate_info(conn, (struct validate_negotiate_info_req *)&req->Buffer[0], - (struct validate_negotiate_info_rsp *)&rsp->Buffer[0]); + (struct validate_negotiate_info_rsp *)&rsp->Buffer[0], + in_buf_len); if (ret < 0) goto out; @@ -7476,7 +7492,7 @@ int smb2_ioctl(struct ksmbd_work *work) rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID); break; case FSCTL_QUERY_NETWORK_INTERFACE_INFO: - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp); + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len); if (nbytes < 0) goto out; break; @@ -7503,15 +7519,33 @@ int smb2_ioctl(struct ksmbd_work *work) goto out; } + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) { + ret = -EINVAL; + goto out; + } + if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) { ret = -EINVAL; goto out; } nbytes = sizeof(struct copychunk_ioctl_rsp); - fsctl_copychunk(work, req, rsp); + rsp->VolatileFileId = req->VolatileFileId; + rsp->PersistentFileId = req->PersistentFileId; + fsctl_copychunk(work, + (struct copychunk_ioctl_req *)&req->Buffer[0], + le32_to_cpu(req->CntCode), + le32_to_cpu(req->InputCount), + le64_to_cpu(req->VolatileFileId), + le64_to_cpu(req->PersistentFileId), + rsp); break; case FSCTL_SET_SPARSE: + if (in_buf_len < sizeof(struct file_sparse)) { + ret = -EINVAL; + goto out; + } + ret = fsctl_set_sparse(work, id, (struct file_sparse *)&req->Buffer[0]); if (ret < 0) @@ -7530,6 +7564,11 @@ int smb2_ioctl(struct ksmbd_work *work) goto out; } + if (in_buf_len < sizeof(struct file_zero_data_information)) { + ret = -EINVAL; + goto out; + } + zero_data = (struct file_zero_data_information *)&req->Buffer[0]; @@ -7549,6 +7588,11 @@ int smb2_ioctl(struct ksmbd_work *work) break; } case FSCTL_QUERY_ALLOCATED_RANGES: + if (in_buf_len < sizeof(struct file_allocated_range_buffer)) { + ret = -EINVAL; + goto out; + } + ret = fsctl_query_allocated_ranges(work, id, (struct file_allocated_range_buffer *)&req->Buffer[0], (struct file_allocated_range_buffer *)&rsp->Buffer[0], @@ -7589,6 +7633,11 @@ int smb2_ioctl(struct ksmbd_work *work) struct duplicate_extents_to_file *dup_ext; loff_t src_off, dst_off, length, cloned; + if (in_buf_len < sizeof(struct duplicate_extents_to_file)) { + ret = -EINVAL; + goto out; + } + dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0]; fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle,
Add validation for request/response buffer size check in smb2_ioctl and fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req argument of fsctl_validate_negotiate_info. Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Ralph Böhme <slow@samba.org> Cc: Steve French <smfrench@gmail.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- v2: - fix warning: variable 'ret' is used uninitialized ret. v3: - fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments instead of smb2_ioctl_req structure. - remove an unused smb2_ioctl_req argument of fsctl_validate_negotiate_info. fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 68 insertions(+), 19 deletions(-)