2021-09-22 13:08 GMT+09:00, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>: > 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@xxxxxxxxx> 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@xxxxxxxxxx> >> 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@xxxxxxxxx> >> > Cc: Ralph Böhme <slow@xxxxxxxxx> >> > Cc: Steve French <smfrench@xxxxxxxxx> >> > Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx> >> > --- >> > 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 >> > >
