Re: [PATCH v3] ksmbd: add validation in smb2_ioctl

[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?

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
>>>>
>>
>