* [bug report] ksmbd: check invalid FileOffset and BeyondFinalZero in FSCTL_ZERO_DATA
@ 2023-03-03 14:12 Dan Carpenter
2023-03-05 12:36 ` Namjae Jeon
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2023-03-03 14:12 UTC (permalink / raw)
To: linkinjeon; +Cc: linux-cifs, cip-dev
Hello Namjae Jeon,
The patch b5e5f9dfc915: "ksmbd: check invalid FileOffset and
BeyondFinalZero in FSCTL_ZERO_DATA" from Jun 19, 2022, leads to the
following Smatch static checker warning:
fs/ksmbd/smb2pdu.c:7759 smb2_ioctl()
warn: no lower bound on 'off'
fs/ksmbd/smb2pdu.c
7573 int smb2_ioctl(struct ksmbd_work *work)
7574 {
7575 struct smb2_ioctl_req *req;
7576 struct smb2_ioctl_rsp *rsp;
7577 unsigned int cnt_code, nbytes = 0, out_buf_len, in_buf_len;
7578 u64 id = KSMBD_NO_FID;
7579 struct ksmbd_conn *conn = work->conn;
7580 int ret = 0;
7581
7582 if (work->next_smb2_rcv_hdr_off) {
7583 req = ksmbd_req_buf_next(work);
7584 rsp = ksmbd_resp_buf_next(work);
7585 if (!has_file_id(req->VolatileFileId)) {
7586 ksmbd_debug(SMB, "Compound request set FID = %llu\n",
7587 work->compound_fid);
7588 id = work->compound_fid;
7589 }
7590 } else {
7591 req = smb2_get_msg(work->request_buf);
7592 rsp = smb2_get_msg(work->response_buf);
7593 }
7594
7595 if (!has_file_id(id))
7596 id = req->VolatileFileId;
7597
7598 if (req->Flags != cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL)) {
7599 rsp->hdr.Status = STATUS_NOT_SUPPORTED;
7600 goto out;
7601 }
7602
7603 cnt_code = le32_to_cpu(req->CtlCode);
7604 ret = smb2_calc_max_out_buf_len(work, 48,
7605 le32_to_cpu(req->MaxOutputResponse));
7606 if (ret < 0) {
7607 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
7608 goto out;
7609 }
7610 out_buf_len = (unsigned int)ret;
7611 in_buf_len = le32_to_cpu(req->InputCount);
7612
7613 switch (cnt_code) {
7614 case FSCTL_DFS_GET_REFERRALS:
7615 case FSCTL_DFS_GET_REFERRALS_EX:
7616 /* Not support DFS yet */
7617 rsp->hdr.Status = STATUS_FS_DRIVER_REQUIRED;
7618 goto out;
7619 case FSCTL_CREATE_OR_GET_OBJECT_ID:
7620 {
7621 struct file_object_buf_type1_ioctl_rsp *obj_buf;
7622
7623 nbytes = sizeof(struct file_object_buf_type1_ioctl_rsp);
7624 obj_buf = (struct file_object_buf_type1_ioctl_rsp *)
7625 &rsp->Buffer[0];
7626
7627 /*
7628 * TODO: This is dummy implementation to pass smbtorture
7629 * Need to check correct response later
7630 */
7631 memset(obj_buf->ObjectId, 0x0, 16);
7632 memset(obj_buf->BirthVolumeId, 0x0, 16);
7633 memset(obj_buf->BirthObjectId, 0x0, 16);
7634 memset(obj_buf->DomainId, 0x0, 16);
7635
7636 break;
7637 }
7638 case FSCTL_PIPE_TRANSCEIVE:
7639 out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
7640 nbytes = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
7641 break;
7642 case FSCTL_VALIDATE_NEGOTIATE_INFO:
7643 if (conn->dialect < SMB30_PROT_ID) {
7644 ret = -EOPNOTSUPP;
7645 goto out;
7646 }
7647
7648 if (in_buf_len < offsetof(struct validate_negotiate_info_req,
7649 Dialects)) {
7650 ret = -EINVAL;
7651 goto out;
7652 }
7653
7654 if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) {
7655 ret = -EINVAL;
7656 goto out;
7657 }
7658
7659 ret = fsctl_validate_negotiate_info(conn,
7660 (struct validate_negotiate_info_req *)&req->Buffer[0],
7661 (struct validate_negotiate_info_rsp *)&rsp->Buffer[0],
7662 in_buf_len);
7663 if (ret < 0)
7664 goto out;
7665
7666 nbytes = sizeof(struct validate_negotiate_info_rsp);
7667 rsp->PersistentFileId = SMB2_NO_FID;
7668 rsp->VolatileFileId = SMB2_NO_FID;
7669 break;
7670 case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
7671 ret = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
7672 if (ret < 0)
7673 goto out;
7674 nbytes = ret;
7675 break;
7676 case FSCTL_REQUEST_RESUME_KEY:
7677 if (out_buf_len < sizeof(struct resume_key_ioctl_rsp)) {
7678 ret = -EINVAL;
7679 goto out;
7680 }
7681
7682 ret = fsctl_request_resume_key(work, req,
7683 (struct resume_key_ioctl_rsp *)&rsp->Buffer[0]);
7684 if (ret < 0)
7685 goto out;
7686 rsp->PersistentFileId = req->PersistentFileId;
7687 rsp->VolatileFileId = req->VolatileFileId;
7688 nbytes = sizeof(struct resume_key_ioctl_rsp);
7689 break;
7690 case FSCTL_COPYCHUNK:
7691 case FSCTL_COPYCHUNK_WRITE:
7692 if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
7693 ksmbd_debug(SMB,
7694 "User does not have write permission\n");
7695 ret = -EACCES;
7696 goto out;
7697 }
7698
7699 if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
7700 ret = -EINVAL;
7701 goto out;
7702 }
7703
7704 if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) {
7705 ret = -EINVAL;
7706 goto out;
7707 }
7708
7709 nbytes = sizeof(struct copychunk_ioctl_rsp);
7710 rsp->VolatileFileId = req->VolatileFileId;
7711 rsp->PersistentFileId = req->PersistentFileId;
7712 fsctl_copychunk(work,
7713 (struct copychunk_ioctl_req *)&req->Buffer[0],
7714 le32_to_cpu(req->CtlCode),
7715 le32_to_cpu(req->InputCount),
7716 req->VolatileFileId,
7717 req->PersistentFileId,
7718 rsp);
7719 break;
7720 case FSCTL_SET_SPARSE:
7721 if (in_buf_len < sizeof(struct file_sparse)) {
7722 ret = -EINVAL;
7723 goto out;
7724 }
7725
7726 ret = fsctl_set_sparse(work, id,
7727 (struct file_sparse *)&req->Buffer[0]);
7728 if (ret < 0)
7729 goto out;
7730 break;
7731 case FSCTL_SET_ZERO_DATA:
7732 {
7733 struct file_zero_data_information *zero_data;
7734 struct ksmbd_file *fp;
7735 loff_t off, len, bfz;
7736
7737 if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
7738 ksmbd_debug(SMB,
7739 "User does not have write permission\n");
7740 ret = -EACCES;
7741 goto out;
7742 }
7743
7744 if (in_buf_len < sizeof(struct file_zero_data_information)) {
7745 ret = -EINVAL;
7746 goto out;
7747 }
7748
7749 zero_data =
7750 (struct file_zero_data_information *)&req->Buffer[0];
7751
7752 off = le64_to_cpu(zero_data->FileOffset);
7753 bfz = le64_to_cpu(zero_data->BeyondFinalZero);
7754 if (off > bfz) {
Should we check for negative values of "off"? To be honest,
Smatch doesn't really trust either off or bfz...
7755 ret = -EINVAL;
7756 goto out;
7757 }
7758
--> 7759 len = bfz - off;
7760 if (len) {
7761 fp = ksmbd_lookup_fd_fast(work, id);
7762 if (!fp) {
7763 ret = -ENOENT;
7764 goto out;
7765 }
7766
7767 ret = ksmbd_vfs_zero_data(work, fp, off, len);
7768 ksmbd_fd_put(work, fp);
7769 if (ret < 0)
7770 goto out;
7771 }
7772 break;
7773 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] ksmbd: check invalid FileOffset and BeyondFinalZero in FSCTL_ZERO_DATA
2023-03-03 14:12 [bug report] ksmbd: check invalid FileOffset and BeyondFinalZero in FSCTL_ZERO_DATA Dan Carpenter
@ 2023-03-05 12:36 ` Namjae Jeon
0 siblings, 0 replies; 2+ messages in thread
From: Namjae Jeon @ 2023-03-05 12:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-cifs, cip-dev
2023-03-03 23:12 GMT+09:00, Dan Carpenter <error27@gmail.com>:
> Hello Namjae Jeon,
Hello Dan,
I have sent the patch for this to the list.
Thanks for your report!
>
> The patch b5e5f9dfc915: "ksmbd: check invalid FileOffset and
> BeyondFinalZero in FSCTL_ZERO_DATA" from Jun 19, 2022, leads to the
> following Smatch static checker warning:
>
> fs/ksmbd/smb2pdu.c:7759 smb2_ioctl()
> warn: no lower bound on 'off'
>
> fs/ksmbd/smb2pdu.c
> 7573 int smb2_ioctl(struct ksmbd_work *work)
> 7574 {
> 7575 struct smb2_ioctl_req *req;
> 7576 struct smb2_ioctl_rsp *rsp;
> 7577 unsigned int cnt_code, nbytes = 0, out_buf_len,
> in_buf_len;
> 7578 u64 id = KSMBD_NO_FID;
> 7579 struct ksmbd_conn *conn = work->conn;
> 7580 int ret = 0;
> 7581
> 7582 if (work->next_smb2_rcv_hdr_off) {
> 7583 req = ksmbd_req_buf_next(work);
> 7584 rsp = ksmbd_resp_buf_next(work);
> 7585 if (!has_file_id(req->VolatileFileId)) {
> 7586 ksmbd_debug(SMB, "Compound request set FID
> = %llu\n",
> 7587 work->compound_fid);
> 7588 id = work->compound_fid;
> 7589 }
> 7590 } else {
> 7591 req = smb2_get_msg(work->request_buf);
> 7592 rsp = smb2_get_msg(work->response_buf);
> 7593 }
> 7594
> 7595 if (!has_file_id(id))
> 7596 id = req->VolatileFileId;
> 7597
> 7598 if (req->Flags != cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL)) {
> 7599 rsp->hdr.Status = STATUS_NOT_SUPPORTED;
> 7600 goto out;
> 7601 }
> 7602
> 7603 cnt_code = le32_to_cpu(req->CtlCode);
> 7604 ret = smb2_calc_max_out_buf_len(work, 48,
> 7605
> le32_to_cpu(req->MaxOutputResponse));
> 7606 if (ret < 0) {
> 7607 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> 7608 goto out;
> 7609 }
> 7610 out_buf_len = (unsigned int)ret;
> 7611 in_buf_len = le32_to_cpu(req->InputCount);
> 7612
> 7613 switch (cnt_code) {
> 7614 case FSCTL_DFS_GET_REFERRALS:
> 7615 case FSCTL_DFS_GET_REFERRALS_EX:
> 7616 /* Not support DFS yet */
> 7617 rsp->hdr.Status = STATUS_FS_DRIVER_REQUIRED;
> 7618 goto out;
> 7619 case FSCTL_CREATE_OR_GET_OBJECT_ID:
> 7620 {
> 7621 struct file_object_buf_type1_ioctl_rsp *obj_buf;
> 7622
> 7623 nbytes = sizeof(struct
> file_object_buf_type1_ioctl_rsp);
> 7624 obj_buf = (struct file_object_buf_type1_ioctl_rsp
> *)
> 7625 &rsp->Buffer[0];
> 7626
> 7627 /*
> 7628 * TODO: This is dummy implementation to pass
> smbtorture
> 7629 * Need to check correct response later
> 7630 */
> 7631 memset(obj_buf->ObjectId, 0x0, 16);
> 7632 memset(obj_buf->BirthVolumeId, 0x0, 16);
> 7633 memset(obj_buf->BirthObjectId, 0x0, 16);
> 7634 memset(obj_buf->DomainId, 0x0, 16);
> 7635
> 7636 break;
> 7637 }
> 7638 case FSCTL_PIPE_TRANSCEIVE:
> 7639 out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD,
> out_buf_len);
> 7640 nbytes = fsctl_pipe_transceive(work, id,
> out_buf_len, req, rsp);
> 7641 break;
> 7642 case FSCTL_VALIDATE_NEGOTIATE_INFO:
> 7643 if (conn->dialect < SMB30_PROT_ID) {
> 7644 ret = -EOPNOTSUPP;
> 7645 goto out;
> 7646 }
> 7647
> 7648 if (in_buf_len < offsetof(struct
> validate_negotiate_info_req,
> 7649 Dialects)) {
> 7650 ret = -EINVAL;
> 7651 goto out;
> 7652 }
> 7653
> 7654 if (out_buf_len < sizeof(struct
> validate_negotiate_info_rsp)) {
> 7655 ret = -EINVAL;
> 7656 goto out;
> 7657 }
> 7658
> 7659 ret = fsctl_validate_negotiate_info(conn,
> 7660 (struct validate_negotiate_info_req
> *)&req->Buffer[0],
> 7661 (struct validate_negotiate_info_rsp
> *)&rsp->Buffer[0],
> 7662 in_buf_len);
> 7663 if (ret < 0)
> 7664 goto out;
> 7665
> 7666 nbytes = sizeof(struct
> validate_negotiate_info_rsp);
> 7667 rsp->PersistentFileId = SMB2_NO_FID;
> 7668 rsp->VolatileFileId = SMB2_NO_FID;
> 7669 break;
> 7670 case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
> 7671 ret = fsctl_query_iface_info_ioctl(conn, rsp,
> out_buf_len);
> 7672 if (ret < 0)
> 7673 goto out;
> 7674 nbytes = ret;
> 7675 break;
> 7676 case FSCTL_REQUEST_RESUME_KEY:
> 7677 if (out_buf_len < sizeof(struct
> resume_key_ioctl_rsp)) {
> 7678 ret = -EINVAL;
> 7679 goto out;
> 7680 }
> 7681
> 7682 ret = fsctl_request_resume_key(work, req,
> 7683 (struct
> resume_key_ioctl_rsp *)&rsp->Buffer[0]);
> 7684 if (ret < 0)
> 7685 goto out;
> 7686 rsp->PersistentFileId = req->PersistentFileId;
> 7687 rsp->VolatileFileId = req->VolatileFileId;
> 7688 nbytes = sizeof(struct resume_key_ioctl_rsp);
> 7689 break;
> 7690 case FSCTL_COPYCHUNK:
> 7691 case FSCTL_COPYCHUNK_WRITE:
> 7692 if (!test_tree_conn_flag(work->tcon,
> KSMBD_TREE_CONN_FLAG_WRITABLE)) {
> 7693 ksmbd_debug(SMB,
> 7694 "User does not have write
> permission\n");
> 7695 ret = -EACCES;
> 7696 goto out;
> 7697 }
> 7698
> 7699 if (in_buf_len < sizeof(struct
> copychunk_ioctl_req)) {
> 7700 ret = -EINVAL;
> 7701 goto out;
> 7702 }
> 7703
> 7704 if (out_buf_len < sizeof(struct
> copychunk_ioctl_rsp)) {
> 7705 ret = -EINVAL;
> 7706 goto out;
> 7707 }
> 7708
> 7709 nbytes = sizeof(struct copychunk_ioctl_rsp);
> 7710 rsp->VolatileFileId = req->VolatileFileId;
> 7711 rsp->PersistentFileId = req->PersistentFileId;
> 7712 fsctl_copychunk(work,
> 7713 (struct copychunk_ioctl_req
> *)&req->Buffer[0],
> 7714 le32_to_cpu(req->CtlCode),
> 7715 le32_to_cpu(req->InputCount),
> 7716 req->VolatileFileId,
> 7717 req->PersistentFileId,
> 7718 rsp);
> 7719 break;
> 7720 case FSCTL_SET_SPARSE:
> 7721 if (in_buf_len < sizeof(struct file_sparse)) {
> 7722 ret = -EINVAL;
> 7723 goto out;
> 7724 }
> 7725
> 7726 ret = fsctl_set_sparse(work, id,
> 7727 (struct file_sparse
> *)&req->Buffer[0]);
> 7728 if (ret < 0)
> 7729 goto out;
> 7730 break;
> 7731 case FSCTL_SET_ZERO_DATA:
> 7732 {
> 7733 struct file_zero_data_information *zero_data;
> 7734 struct ksmbd_file *fp;
> 7735 loff_t off, len, bfz;
> 7736
> 7737 if (!test_tree_conn_flag(work->tcon,
> KSMBD_TREE_CONN_FLAG_WRITABLE)) {
> 7738 ksmbd_debug(SMB,
> 7739 "User does not have write
> permission\n");
> 7740 ret = -EACCES;
> 7741 goto out;
> 7742 }
> 7743
> 7744 if (in_buf_len < sizeof(struct
> file_zero_data_information)) {
> 7745 ret = -EINVAL;
> 7746 goto out;
> 7747 }
> 7748
> 7749 zero_data =
> 7750 (struct file_zero_data_information
> *)&req->Buffer[0];
> 7751
> 7752 off = le64_to_cpu(zero_data->FileOffset);
> 7753 bfz = le64_to_cpu(zero_data->BeyondFinalZero);
> 7754 if (off > bfz) {
>
> Should we check for negative values of "off"? To be honest,
> Smatch doesn't really trust either off or bfz...
>
> 7755 ret = -EINVAL;
> 7756 goto out;
> 7757 }
> 7758
> --> 7759 len = bfz - off;
> 7760 if (len) {
> 7761 fp = ksmbd_lookup_fd_fast(work, id);
> 7762 if (!fp) {
> 7763 ret = -ENOENT;
> 7764 goto out;
> 7765 }
> 7766
> 7767 ret = ksmbd_vfs_zero_data(work, fp, off,
> len);
> 7768 ksmbd_fd_put(work, fp);
> 7769 if (ret < 0)
> 7770 goto out;
> 7771 }
> 7772 break;
> 7773 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-03-05 22:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-03 14:12 [bug report] ksmbd: check invalid FileOffset and BeyondFinalZero in FSCTL_ZERO_DATA Dan Carpenter
2023-03-05 12:36 ` Namjae Jeon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).