All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] ksmbd: add validation in smb2_ioctl
@ 2021-09-22  2:28 Namjae Jeon
  2021-09-22  3:48 ` ronnie sahlberg
  0 siblings, 1 reply; 6+ messages in thread
From: Namjae Jeon @ 2021-09-22  2:28 UTC (permalink / raw)
  To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ksmbd: add validation in smb2_ioctl
  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
  0 siblings, 1 reply; 6+ messages in thread
From: ronnie sahlberg @ 2021-09-22  3:48 UTC (permalink / raw)
  To: Namjae Jeon; +Cc: linux-cifs, Ralph Böhme, Steve French

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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ksmbd: add validation in smb2_ioctl
  2021-09-22  3:48 ` ronnie sahlberg
@ 2021-09-22  4:08   ` ronnie sahlberg
  2021-09-22  4:13     ` Namjae Jeon
  0 siblings, 1 reply; 6+ messages in thread
From: ronnie sahlberg @ 2021-09-22  4:08 UTC (permalink / raw)
  To: Namjae Jeon; +Cc: linux-cifs, Ralph Böhme, Steve French

Ignore.  Looking at the wrong code.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ksmbd: add validation in smb2_ioctl
  2021-09-22  4:08   ` ronnie sahlberg
@ 2021-09-22  4:13     ` Namjae Jeon
  2021-09-23 15:02       ` Tom Talpey
  0 siblings, 1 reply; 6+ messages in thread
From: Namjae Jeon @ 2021-09-22  4:13 UTC (permalink / raw)
  To: ronnie sahlberg; +Cc: linux-cifs, Ralph Böhme, Steve French

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ksmbd: add validation in smb2_ioctl
  2021-09-22  4:13     ` Namjae Jeon
@ 2021-09-23 15:02       ` Tom Talpey
  2021-09-23 19:04         ` Namjae Jeon
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Talpey @ 2021-09-23 15:02 UTC (permalink / raw)
  To: Namjae Jeon, ronnie sahlberg; +Cc: linux-cifs, Ralph Böhme, Steve French

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ksmbd: add validation in smb2_ioctl
  2021-09-23 15:02       ` Tom Talpey
@ 2021-09-23 19:04         ` Namjae Jeon
  0 siblings, 0 replies; 6+ messages in thread
From: Namjae Jeon @ 2021-09-23 19:04 UTC (permalink / raw)
  To: Tom Talpey; +Cc: ronnie sahlberg, linux-cifs, Ralph Böhme, Steve French

2021-09-24 0:02 GMT+09:00, 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?
I have sent v3 patch to the list before, If you don't look for it, I
will re-send the patch to list.
>
> 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
>>>>>
>>>
>>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-23 19:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2021-09-23 15:02       ` Tom Talpey
2021-09-23 19:04         ` Namjae Jeon

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.