* [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 related [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.