| Message ID | 20210918094513.89480-2-linkinjeon@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] ksmbd: add request buffer validation in smb2_set_info | expand |
Hi Namjae,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.15-rc1 next-20210917]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Namjae-Jeon/ksmbd-add-request-buffer-validation-in-smb2_set_info/20210918-174717
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4357f03d6611753936e4d52fc251b54a6afb1b54
config: hexagon-randconfig-r022-20210918 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c8b3d7d6d6de37af68b2f379d0e37304f78e115f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/57e7ede2bf2d38cb0f368f2fc54d646168b3d119
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Namjae-Jeon/ksmbd-add-request-buffer-validation-in-smb2_set_info/20210918-174717
git checkout 57e7ede2bf2d38cb0f368f2fc54d646168b3d119
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> fs/ksmbd/smb2pdu.c:7037:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (chunk_count == 0)
^~~~~~~~~~~~~~~~
fs/ksmbd/smb2pdu.c:7120:9: note: uninitialized use occurs here
return ret;
^~~
fs/ksmbd/smb2pdu.c:7037:2: note: remove the 'if' if its condition is always false
if (chunk_count == 0)
^~~~~~~~~~~~~~~~~~~~~
fs/ksmbd/smb2pdu.c:7020:9: note: initialize the variable 'ret' to silence this warning
int ret, cnt_code;
^
= 0
1 warning generated.
vim +7037 fs/ksmbd/smb2pdu.c
7009
7010 static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
7011 struct smb2_ioctl_rsp *rsp)
7012 {
7013 struct copychunk_ioctl_req *ci_req;
7014 struct copychunk_ioctl_rsp *ci_rsp;
7015 struct ksmbd_file *src_fp = NULL, *dst_fp = NULL;
7016 struct srv_copychunk *chunks;
7017 unsigned int i, chunk_count, chunk_count_written = 0;
7018 unsigned int chunk_size_written = 0;
7019 loff_t total_size_written = 0;
7020 int ret, cnt_code;
7021
7022 cnt_code = le32_to_cpu(req->CntCode);
7023 ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
7024 ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0];
7025
7026 rsp->VolatileFileId = req->VolatileFileId;
7027 rsp->PersistentFileId = req->PersistentFileId;
7028 ci_rsp->ChunksWritten =
7029 cpu_to_le32(ksmbd_server_side_copy_max_chunk_count());
7030 ci_rsp->ChunkBytesWritten =
7031 cpu_to_le32(ksmbd_server_side_copy_max_chunk_size());
7032 ci_rsp->TotalBytesWritten =
7033 cpu_to_le32(ksmbd_server_side_copy_max_total_size());
7034
7035 chunks = (struct srv_copychunk *)&ci_req->Chunks[0];
7036 chunk_count = le32_to_cpu(ci_req->ChunkCount);
> 7037 if (chunk_count == 0)
7038 goto out;
7039 total_size_written = 0;
7040
7041 /* verify the SRV_COPYCHUNK_COPY packet */
7042 if (chunk_count > ksmbd_server_side_copy_max_chunk_count() ||
7043 le32_to_cpu(req->InputCount) <
7044 offsetof(struct copychunk_ioctl_req, Chunks) +
7045 chunk_count * sizeof(struct srv_copychunk)) {
7046 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
7047 return -EINVAL;
7048 }
7049
7050 for (i = 0; i < chunk_count; i++) {
7051 if (le32_to_cpu(chunks[i].Length) == 0 ||
7052 le32_to_cpu(chunks[i].Length) > ksmbd_server_side_copy_max_chunk_size())
7053 break;
7054 total_size_written += le32_to_cpu(chunks[i].Length);
7055 }
7056
7057 if (i < chunk_count ||
7058 total_size_written > ksmbd_server_side_copy_max_total_size()) {
7059 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
7060 return -EINVAL;
7061 }
7062
7063 src_fp = ksmbd_lookup_foreign_fd(work,
7064 le64_to_cpu(ci_req->ResumeKey[0]));
7065 dst_fp = ksmbd_lookup_fd_slow(work,
7066 le64_to_cpu(req->VolatileFileId),
7067 le64_to_cpu(req->PersistentFileId));
7068 ret = -EINVAL;
7069 if (!src_fp ||
7070 src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) {
7071 rsp->hdr.Status = STATUS_OBJECT_NAME_NOT_FOUND;
7072 goto out;
7073 }
7074
7075 if (!dst_fp) {
7076 rsp->hdr.Status = STATUS_FILE_CLOSED;
7077 goto out;
7078 }
7079
7080 /*
7081 * FILE_READ_DATA should only be included in
7082 * the FSCTL_COPYCHUNK case
7083 */
7084 if (cnt_code == FSCTL_COPYCHUNK &&
7085 !(dst_fp->daccess & (FILE_READ_DATA_LE | FILE_GENERIC_READ_LE))) {
7086 rsp->hdr.Status = STATUS_ACCESS_DENIED;
7087 goto out;
7088 }
7089
7090 ret = ksmbd_vfs_copy_file_ranges(work, src_fp, dst_fp,
7091 chunks, chunk_count,
7092 &chunk_count_written,
7093 &chunk_size_written,
7094 &total_size_written);
7095 if (ret < 0) {
7096 if (ret == -EACCES)
7097 rsp->hdr.Status = STATUS_ACCESS_DENIED;
7098 if (ret == -EAGAIN)
7099 rsp->hdr.Status = STATUS_FILE_LOCK_CONFLICT;
7100 else if (ret == -EBADF)
7101 rsp->hdr.Status = STATUS_INVALID_HANDLE;
7102 else if (ret == -EFBIG || ret == -ENOSPC)
7103 rsp->hdr.Status = STATUS_DISK_FULL;
7104 else if (ret == -EINVAL)
7105 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
7106 else if (ret == -EISDIR)
7107 rsp->hdr.Status = STATUS_FILE_IS_A_DIRECTORY;
7108 else if (ret == -E2BIG)
7109 rsp->hdr.Status = STATUS_INVALID_VIEW_SIZE;
7110 else
7111 rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
7112 }
7113
7114 ci_rsp->ChunksWritten = cpu_to_le32(chunk_count_written);
7115 ci_rsp->ChunkBytesWritten = cpu_to_le32(chunk_size_written);
7116 ci_rsp->TotalBytesWritten = cpu_to_le32(total_size_written);
7117 out:
7118 ksmbd_fd_put(work, src_fp);
7119 ksmbd_fd_put(work, dst_fp);
7120 return ret;
7121 }
7122
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
2021-09-19 3:51 GMT+09:00, kernel test robot <lkp@intel.com>: > Hi Namjae, Hi, I will fix it, Thanks for your report! > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on linus/master] > [also build test WARNING on v5.15-rc1 next-20210917] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: > https://github.com/0day-ci/linux/commits/Namjae-Jeon/ksmbd-add-request-buffer-validation-in-smb2_set_info/20210918-174717 > base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > 4357f03d6611753936e4d52fc251b54a6afb1b54 > config: hexagon-randconfig-r022-20210918 (attached as .config) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project > c8b3d7d6d6de37af68b2f379d0e37304f78e115f) > reproduce (this is a W=1 build): > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > # > https://github.com/0day-ci/linux/commit/57e7ede2bf2d38cb0f368f2fc54d646168b3d119 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review > Namjae-Jeon/ksmbd-add-request-buffer-validation-in-smb2_set_info/20210918-174717 > git checkout 57e7ede2bf2d38cb0f368f2fc54d646168b3d119 > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 > ARCH=hexagon > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > >>> fs/ksmbd/smb2pdu.c:7037:6: warning: variable 'ret' is used uninitialized >>> whenever 'if' condition is true [-Wsometimes-uninitialized] > if (chunk_count == 0) > ^~~~~~~~~~~~~~~~ > fs/ksmbd/smb2pdu.c:7120:9: note: uninitialized use occurs here > return ret; > ^~~ > fs/ksmbd/smb2pdu.c:7037:2: note: remove the 'if' if its condition is > always false > if (chunk_count == 0) > ^~~~~~~~~~~~~~~~~~~~~ > fs/ksmbd/smb2pdu.c:7020:9: note: initialize the variable 'ret' to silence > this warning > int ret, cnt_code; > ^ > = 0 > 1 warning generated. > > > vim +7037 fs/ksmbd/smb2pdu.c > > 7009 > 7010 static int fsctl_copychunk(struct ksmbd_work *work, struct > smb2_ioctl_req *req, > 7011 struct smb2_ioctl_rsp *rsp) > 7012 { > 7013 struct copychunk_ioctl_req *ci_req; > 7014 struct copychunk_ioctl_rsp *ci_rsp; > 7015 struct ksmbd_file *src_fp = NULL, *dst_fp = NULL; > 7016 struct srv_copychunk *chunks; > 7017 unsigned int i, chunk_count, chunk_count_written = 0; > 7018 unsigned int chunk_size_written = 0; > 7019 loff_t total_size_written = 0; > 7020 int ret, cnt_code; > 7021 > 7022 cnt_code = le32_to_cpu(req->CntCode); > 7023 ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0]; > 7024 ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0]; > 7025 > 7026 rsp->VolatileFileId = req->VolatileFileId; > 7027 rsp->PersistentFileId = req->PersistentFileId; > 7028 ci_rsp->ChunksWritten = > 7029 cpu_to_le32(ksmbd_server_side_copy_max_chunk_count()); > 7030 ci_rsp->ChunkBytesWritten = > 7031 cpu_to_le32(ksmbd_server_side_copy_max_chunk_size()); > 7032 ci_rsp->TotalBytesWritten = > 7033 cpu_to_le32(ksmbd_server_side_copy_max_total_size()); > 7034 > 7035 chunks = (struct srv_copychunk *)&ci_req->Chunks[0]; > 7036 chunk_count = le32_to_cpu(ci_req->ChunkCount); >> 7037 if (chunk_count == 0) > 7038 goto out; > 7039 total_size_written = 0; > 7040 > 7041 /* verify the SRV_COPYCHUNK_COPY packet */ > 7042 if (chunk_count > ksmbd_server_side_copy_max_chunk_count() || > 7043 le32_to_cpu(req->InputCount) < > 7044 offsetof(struct copychunk_ioctl_req, Chunks) + > 7045 chunk_count * sizeof(struct srv_copychunk)) { > 7046 rsp->hdr.Status = STATUS_INVALID_PARAMETER; > 7047 return -EINVAL; > 7048 } > 7049 > 7050 for (i = 0; i < chunk_count; i++) { > 7051 if (le32_to_cpu(chunks[i].Length) == 0 || > 7052 le32_to_cpu(chunks[i].Length) > > ksmbd_server_side_copy_max_chunk_size()) > 7053 break; > 7054 total_size_written += le32_to_cpu(chunks[i].Length); > 7055 } > 7056 > 7057 if (i < chunk_count || > 7058 total_size_written > ksmbd_server_side_copy_max_total_size()) { > 7059 rsp->hdr.Status = STATUS_INVALID_PARAMETER; > 7060 return -EINVAL; > 7061 } > 7062 > 7063 src_fp = ksmbd_lookup_foreign_fd(work, > 7064 le64_to_cpu(ci_req->ResumeKey[0])); > 7065 dst_fp = ksmbd_lookup_fd_slow(work, > 7066 le64_to_cpu(req->VolatileFileId), > 7067 le64_to_cpu(req->PersistentFileId)); > 7068 ret = -EINVAL; > 7069 if (!src_fp || > 7070 src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) { > 7071 rsp->hdr.Status = STATUS_OBJECT_NAME_NOT_FOUND; > 7072 goto out; > 7073 } > 7074 > 7075 if (!dst_fp) { > 7076 rsp->hdr.Status = STATUS_FILE_CLOSED; > 7077 goto out; > 7078 } > 7079 > 7080 /* > 7081 * FILE_READ_DATA should only be included in > 7082 * the FSCTL_COPYCHUNK case > 7083 */ > 7084 if (cnt_code == FSCTL_COPYCHUNK && > 7085 !(dst_fp->daccess & (FILE_READ_DATA_LE | FILE_GENERIC_READ_LE))) > { > 7086 rsp->hdr.Status = STATUS_ACCESS_DENIED; > 7087 goto out; > 7088 } > 7089 > 7090 ret = ksmbd_vfs_copy_file_ranges(work, src_fp, dst_fp, > 7091 chunks, chunk_count, > 7092 &chunk_count_written, > 7093 &chunk_size_written, > 7094 &total_size_written); > 7095 if (ret < 0) { > 7096 if (ret == -EACCES) > 7097 rsp->hdr.Status = STATUS_ACCESS_DENIED; > 7098 if (ret == -EAGAIN) > 7099 rsp->hdr.Status = STATUS_FILE_LOCK_CONFLICT; > 7100 else if (ret == -EBADF) > 7101 rsp->hdr.Status = STATUS_INVALID_HANDLE; > 7102 else if (ret == -EFBIG || ret == -ENOSPC) > 7103 rsp->hdr.Status = STATUS_DISK_FULL; > 7104 else if (ret == -EINVAL) > 7105 rsp->hdr.Status = STATUS_INVALID_PARAMETER; > 7106 else if (ret == -EISDIR) > 7107 rsp->hdr.Status = STATUS_FILE_IS_A_DIRECTORY; > 7108 else if (ret == -E2BIG) > 7109 rsp->hdr.Status = STATUS_INVALID_VIEW_SIZE; > 7110 else > 7111 rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR; > 7112 } > 7113 > 7114 ci_rsp->ChunksWritten = cpu_to_le32(chunk_count_written); > 7115 ci_rsp->ChunkBytesWritten = cpu_to_le32(chunk_size_written); > 7116 ci_rsp->TotalBytesWritten = cpu_to_le32(total_size_written); > 7117 out: > 7118 ksmbd_fd_put(work, src_fp); > 7119 ksmbd_fd_put(work, dst_fp); > 7120 return ret; > 7121 } > 7122 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index 7763f69e1ae8..e589e8cc389f 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -7038,6 +7038,8 @@ 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 */ @@ -7142,7 +7144,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; @@ -7225,6 +7228,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(); @@ -7245,11 +7250,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) { @@ -7425,7 +7435,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; @@ -7455,6 +7465,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: @@ -7490,9 +7501,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; @@ -7501,7 +7519,8 @@ 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, req, rsp, + out_buf_len); if (nbytes < 0) goto out; break; @@ -7528,6 +7547,11 @@ 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; @@ -7537,6 +7561,11 @@ int smb2_ioctl(struct ksmbd_work *work) fsctl_copychunk(work, req, 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) @@ -7555,6 +7584,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]; @@ -7574,6 +7608,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], @@ -7614,6 +7653,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,
Add validation for request/response buffer size check in smb2_ioctl. 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> --- fs/ksmbd/smb2pdu.c | 54 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 5 deletions(-)