From: Namjae Jeon <linkinjeon@kernel.org>
To: Hyunchul Lee <hyc.lee@gmail.com>
Cc: linux-cifs <linux-cifs@vger.kernel.org>,
"Tom Talpey" <tom@talpey.com>,
"Ronnie Sahlberg" <ronniesahlberg@gmail.com>,
"Ralph Böhme" <slow@samba.org>,
"Steve French" <smfrench@gmail.com>,
"Sergey Senozhatsky" <senozhatsky@chromium.org>
Subject: Re: [PATCH 1/7] ksmbd: add validation in smb2_ioctl
Date: Sat, 25 Sep 2021 19:44:05 +0900 [thread overview]
Message-ID: <CAKYAXd-buj=iLH8VsKoMuYR7k-UAfk+_AaHNaT9OwENSGcaxew@mail.gmail.com> (raw)
In-Reply-To: <CANFS6baQVtiBGQCGeZhvCrPeSVF0647PzsNRrU93Wra+m_iNEg@mail.gmail.com>
2021-09-25 19:16 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> 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: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> 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 0c49a0e887d3..b22d4207c077 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -6927,24 +6927,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 =
>> @@ -6954,12 +6956,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;
>> @@ -6980,9 +6983,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])) {
>> @@ -7057,8 +7058,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;
>> @@ -7141,6 +7142,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;
>
> Do we need to check whether the remaining(len out_buf_len - nbytes) is
> less than sizeof(struct network_interface_info_ioctl_rsp)?
Right. will fix it on next version. Thanks for your review!
>
>> nbytes += sizeof(struct
>> network_interface_info_ioctl_rsp);
>> }
>> rtnl_unlock();
>> @@ -7161,11 +7164,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) {
>> @@ -7341,7 +7349,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;
>> @@ -7371,6 +7379,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:
>> @@ -7406,9 +7415,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;
>>
>> @@ -7417,7 +7433,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;
>> @@ -7444,15 +7460,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)
>> @@ -7471,6 +7505,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];
>>
>> @@ -7490,6 +7529,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],
>> @@ -7530,6 +7574,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
>>
>
>
> --
> Thanks,
> Hyunchul
>
next prev parent reply other threads:[~2021-09-25 10:44 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
2021-09-24 2:12 ` [PATCH 1/7] ksmbd: add validation in smb2_ioctl Namjae Jeon
2021-09-25 10:16 ` Hyunchul Lee
2021-09-25 10:44 ` Namjae Jeon [this message]
2021-09-24 2:12 ` [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
2021-09-25 8:13 ` Hyunchul Lee
2021-09-25 9:19 ` Namjae Jeon
2021-09-24 2:12 ` [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
2021-09-25 8:18 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message() Namjae Jeon
2021-09-25 10:27 ` Hyunchul Lee
2021-09-25 10:46 ` Namjae Jeon
2021-09-24 2:12 ` [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Namjae Jeon
2021-09-25 8:41 ` Hyunchul Lee
2021-09-25 9:24 ` Namjae Jeon
2021-09-24 2:12 ` [PATCH 6/7] ksmbd: fix invalid request buffer access in compound Namjae Jeon
2021-09-25 9:41 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 7/7] ksmbd: add validation in smb2 negotiate Namjae Jeon
2021-09-24 4:58 ` 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-buj=iLH8VsKoMuYR7k-UAfk+_AaHNaT9OwENSGcaxew@mail.gmail.com' \
--to=linkinjeon@kernel.org \
--cc=hyc.lee@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=ronniesahlberg@gmail.com \
--cc=senozhatsky@chromium.org \
--cc=slow@samba.org \
--cc=smfrench@gmail.com \
--cc=tom@talpey.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.