From: Namjae Jeon <linkinjeon@kernel.org>
To: Ralph Boehme <slow@samba.org>
Cc: linux-cifs@vger.kernel.org, Tom Talpey <tom@talpey.com>,
Ronnie Sahlberg <ronniesahlberg@gmail.com>,
Steve French <smfrench@gmail.com>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
Hyunchul Lee <hyc.lee@gmail.com>
Subject: Re: [PATCH v6 02/14] ksmbd: add validation in smb2_ioctl
Date: Sun, 3 Oct 2021 10:21:41 +0900 [thread overview]
Message-ID: <CAKYAXd_Qz6TXcA8eyfaQ4qW7-3M29LixVcWurSnz43r0znJgGQ@mail.gmail.com> (raw)
In-Reply-To: <20211002131212.130629-3-slow@samba.org>
2021-10-02 22:12 GMT+09:00, Ralph Boehme <slow@samba.org>:
> From: 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: Sergey Senozhatsky <senozhatsky@chromium.org>
> Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
If you don't find any issue in this patch, please mark your reviewed-by tag.
Other patches also..
Thanks!
> ---
> fs/ksmbd/smb2pdu.c | 105 ++++++++++++++++++++++++++++++++++-----------
> fs/ksmbd/vfs.c | 2 +-
> fs/ksmbd/vfs.h | 2 +-
> 3 files changed, 83 insertions(+), 26 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index dcf907738610..3476cacd2784 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7044,24 +7044,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 =
> @@ -7071,12 +7073,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;
> @@ -7097,9 +7100,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])) {
> @@ -7174,11 +7175,11 @@ 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,
> + unsigned int out_buf_len)
> {
> struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
> - int nbytes = 0;
> + unsigned int nbytes = 0;
> struct net_device *netdev;
> struct sockaddr_storage_rsp *sockaddr_storage;
> unsigned int flags;
> @@ -7187,6 +7188,10 @@ static int fsctl_query_iface_info_ioctl(struct
> ksmbd_conn *conn,
>
> rtnl_lock();
> for_each_netdev(&init_net, netdev) {
> + if (out_buf_len <
> + nbytes + sizeof(struct network_interface_info_ioctl_rsp))
> + break;
> +
> if (netdev->type == ARPHRD_LOOPBACK)
> continue;
>
> @@ -7258,6 +7263,8 @@ static int fsctl_query_iface_info_ioctl(struct
> ksmbd_conn *conn,
> sockaddr_storage->addr6.ScopeId = 0;
> }
>
> + if (out_buf_len - nbytes < sizeof(struct
> network_interface_info_ioctl_rsp))
> + break;
> nbytes += sizeof(struct network_interface_info_ioctl_rsp);
> }
> rtnl_unlock();
> @@ -7278,11 +7285,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,
> + unsigned 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) {
> @@ -7316,7 +7328,7 @@ static int fsctl_validate_negotiate_info(struct
> ksmbd_conn *conn,
> static int fsctl_query_allocated_ranges(struct ksmbd_work *work, u64 id,
> struct file_allocated_range_buffer *qar_req,
> struct file_allocated_range_buffer *qar_rsp,
> - int in_count, int *out_count)
> + unsigned int in_count, unsigned int *out_count)
> {
> struct ksmbd_file *fp;
> loff_t start, length;
> @@ -7343,7 +7355,8 @@ static int fsctl_query_allocated_ranges(struct
> ksmbd_work *work, u64 id,
> }
>
> static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
> - int out_buf_len, struct smb2_ioctl_req *req,
> + unsigned int out_buf_len,
> + struct smb2_ioctl_req *req,
> struct smb2_ioctl_rsp *rsp)
> {
> struct ksmbd_rpc_command *rpc_resp;
> @@ -7457,8 +7470,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;
> + unsigned int cnt_code, nbytes = 0, out_buf_len, in_buf_len;
> u64 id = KSMBD_NO_FID;
> struct ksmbd_conn *conn = work->conn;
> int ret = 0;
> @@ -7487,7 +7499,11 @@ 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);
> + out_buf_len =
> + min_t(u32, work->response_sz - work->next_smb2_rsp_hdr_off -
> + (offsetof(struct smb2_ioctl_rsp, Buffer) - 4),
> + out_buf_len);
> + in_buf_len = le32_to_cpu(req->InputCount);
>
> switch (cnt_code) {
> case FSCTL_DFS_GET_REFERRALS:
> @@ -7515,6 +7531,7 @@ int smb2_ioctl(struct ksmbd_work *work)
> break;
> }
> case FSCTL_PIPE_TRANSCEIVE:
> + out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
> nbytes = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
> break;
> case FSCTL_VALIDATE_NEGOTIATE_INFO:
> @@ -7523,9 +7540,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;
>
> @@ -7534,7 +7558,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;
> @@ -7561,15 +7585,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)
> @@ -7588,6 +7630,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];
>
> @@ -7607,6 +7654,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],
> @@ -7647,6 +7699,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,
> diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
> index b41954294d38..835b384b0895 100644
> --- a/fs/ksmbd/vfs.c
> +++ b/fs/ksmbd/vfs.c
> @@ -1023,7 +1023,7 @@ int ksmbd_vfs_zero_data(struct ksmbd_work *work,
> struct ksmbd_file *fp,
>
> int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t
> length,
> struct file_allocated_range_buffer *ranges,
> - int in_count, int *out_count)
> + unsigned int in_count, unsigned int *out_count)
> {
> struct file *f = fp->filp;
> struct inode *inode = file_inode(fp->filp);
> diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
> index 7b1dcaa3fbdc..b0d5b8feb4a3 100644
> --- a/fs/ksmbd/vfs.h
> +++ b/fs/ksmbd/vfs.h
> @@ -166,7 +166,7 @@ int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct
> ksmbd_file *fp,
> struct file_allocated_range_buffer;
> int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t
> length,
> struct file_allocated_range_buffer *ranges,
> - int in_count, int *out_count);
> + unsigned int in_count, unsigned int *out_count);
> int ksmbd_vfs_unlink(struct user_namespace *user_ns,
> struct dentry *dir, struct dentry *dentry);
> void *ksmbd_vfs_init_kstat(char **p, struct ksmbd_kstat *ksmbd_kstat);
> --
> 2.31.1
>
>
next prev parent reply other threads:[~2021-10-03 1:21 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-02 13:11 [PATCH v6 00/14] Buffer validation patches Ralph Boehme
2021-10-02 13:11 ` [PATCH v6 01/14] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Ralph Boehme
2021-10-03 1:18 ` Namjae Jeon
2021-10-05 4:24 ` Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 02/14] ksmbd: add validation in smb2_ioctl Ralph Boehme
2021-10-03 1:21 ` Namjae Jeon [this message]
2021-10-02 13:12 ` [PATCH v6 03/14] ksmbd: check strictly data area in ksmbd_smb2_check_message() Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 04/14] ksmbd: remove the leftover of smb2.0 dialect support Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 05/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_verify_smb_message() Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 06/14] ksmbd: check buffer is big enough to access the ProtocolId field Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 07/14] ksmbd: use ksmbd_req_buf_next() in ksmbd_smb2_check_message() Ralph Boehme
2021-10-03 0:59 ` Namjae Jeon
2021-10-05 4:28 ` Ralph Boehme
2021-10-05 18:43 ` Steve French
2021-10-05 19:28 ` Ralph Boehme
2021-10-05 20:20 ` Steve French
2021-10-02 13:12 ` [PATCH v6 08/14] ksmbd: use ksmbd_smb2_cur_pdu_buflen() " Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 09/14] ksmbd: check PDU len is at least header plus body size " Ralph Boehme
2021-10-03 1:11 ` Namjae Jeon
2021-10-05 4:32 ` Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 10/14] ksmdb: use cmd helper variable in smb2_get_ksmbd_tcon() Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 11/14] ksmdb: make smb2_get_ksmbd_tcon() callable with chained PDUs Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 12/14] ksmbd: make smb2_check_user_session() callable for compound PDUs Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 13/14] ksmdb: move session and tcon validation to ksmbd_smb2_check_message() Ralph Boehme
2021-10-02 13:12 ` [PATCH v6 14/14] ksmdb: validate credit charge after validating SMB2 PDU body size Ralph Boehme
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_Qz6TXcA8eyfaQ4qW7-3M29LixVcWurSnz43r0znJgGQ@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.