All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namjae Jeon <linkinjeon@kernel.org>
To: ronnie sahlberg <ronniesahlberg@gmail.com>
Cc: linux-cifs <linux-cifs@vger.kernel.org>,
	"Ralph Böhme" <slow@samba.org>,
	"Steve French" <smfrench@gmail.com>
Subject: Re: [PATCH v3] ksmbd: add validation in smb2_ioctl
Date: Wed, 22 Sep 2021 13:13:40 +0900	[thread overview]
Message-ID: <CAKYAXd-q+6WDZF=stGd+TzmbumTbS7djONqjPUU79EBLUHhzjw@mail.gmail.com> (raw)
In-Reply-To: <CAN05THQxkVzGXZXunqdb-ZpQe+KA0YXjL7tTMKOeEhd_Cv8=Lg@mail.gmail.com>

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

  reply	other threads:[~2021-09-22  4:13 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22  2:28 [PATCH v3] ksmbd: add validation in smb2_ioctl Namjae Jeon
2021-09-22  3:48 ` ronnie sahlberg
2021-09-22  4:08   ` ronnie sahlberg
2021-09-22  4:13     ` Namjae Jeon [this message]
2021-09-23 15:02       ` Tom Talpey
2021-09-23 19:04         ` Namjae Jeon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKYAXd-q+6WDZF=stGd+TzmbumTbS7djONqjPUU79EBLUHhzjw@mail.gmail.com' \
    --to=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.