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