From: Stefan Hajnoczi <stefanha@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: mst@redhat.com, pbonzini@redhat.com, bcodding@redhat.com,
virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Al Viro <viro@zeniv.linux.org.uk>,
Nicholas Bellinger <nab@linux-iscsi.org>
Subject: Re: [PATCH] vhost-scsi: unbreak any layout for response
Date: Wed, 11 Jan 2023 14:18:22 -0500 [thread overview]
Message-ID: <Y78Lfmzr6s1BU3ri@fedora> (raw)
In-Reply-To: <20230111060730.24779-1-jasowang@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6092 bytes --]
On Wed, Jan 11, 2023 at 02:07:30PM +0800, Jason Wang wrote:
> Al Viro said:
>
> """
> Since "vhost/scsi: fix reuse of &vq->iov[out] in response"
> we have this:
> cmd->tvc_resp_iov = vq->iov[vc.out];
> cmd->tvc_in_iovs = vc.in;
> combined with
> iov_iter_init(&iov_iter, ITER_DEST, &cmd->tvc_resp_iov,
> cmd->tvc_in_iovs, sizeof(v_rsp));
> in vhost_scsi_complete_cmd_work(). We used to have ->tvc_resp_iov
> _pointing_ to vq->iov[vc.out]; back then iov_iter_init() asked to
> set an iovec-backed iov_iter over the tail of vq->iov[], with
> length being the amount of iovecs in the tail.
>
> Now we have a copy of one element of that array. Fortunately, the members
> following it in the containing structure are two non-NULL kernel pointers,
> so copy_to_iter() will not copy anything beyond the first iovec - kernel
> pointer is not (on the majority of architectures) going to be accepted by
> access_ok() in copyout() and it won't be skipped since the "length" (in
> reality - another non-NULL kernel pointer) won't be zero.
>
> So it's not going to give a guest-to-qemu escalation, but it's definitely
> a bug. Frankly, my preference would be to verify that the very first iovec
> is long enough to hold rsp_size. Due to the above, any users that try to
> give us vq->iov[vc.out].iov_len < sizeof(struct virtio_scsi_cmd_resp)
> would currently get a failure in vhost_scsi_complete_cmd_work()
> anyway.
> """
>
> However, the spec doesn't say anything about the legacy descriptor
> layout for the respone. So this patch tries to not assume the response
> to reside in a single separate descriptor which is what commit
> 79c14141a487 ("vhost/scsi: Convert completion path to use") tries to
> achieve towards to ANY_LAYOUT.
>
> This is done by allocating and using dedicate resp iov in the
> command. To be safety, start with UIO_MAXIOV to be consistent with the
> vhost core.
>
> Testing with the hacked virtio-scsi driver that use 1 descriptor for 1
> byte in the response.
>
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Benjamin Coddington <bcodding@redhat.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Fixes: a77ec83a5789 ("vhost/scsi: fix reuse of &vq->iov[out] in response")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/scsi.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index dca6346d75b3..7d6d70072603 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -80,7 +80,7 @@ struct vhost_scsi_cmd {
> struct scatterlist *tvc_prot_sgl;
> struct page **tvc_upages;
> /* Pointer to response header iovec */
> - struct iovec tvc_resp_iov;
> + struct iovec *tvc_resp_iov;
> /* Pointer to vhost_scsi for our device */
> struct vhost_scsi *tvc_vhost;
> /* Pointer to vhost_virtqueue for the cmd */
> @@ -563,7 +563,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
> memcpy(v_rsp.sense, cmd->tvc_sense_buf,
> se_cmd->scsi_sense_length);
>
> - iov_iter_init(&iov_iter, ITER_DEST, &cmd->tvc_resp_iov,
> + iov_iter_init(&iov_iter, ITER_DEST, cmd->tvc_resp_iov,
> cmd->tvc_in_iovs, sizeof(v_rsp));
> ret = copy_to_iter(&v_rsp, sizeof(v_rsp), &iov_iter);
> if (likely(ret == sizeof(v_rsp))) {
> @@ -594,6 +594,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> struct vhost_scsi_cmd *cmd;
> struct vhost_scsi_nexus *tv_nexus;
> struct scatterlist *sg, *prot_sg;
> + struct iovec *tvc_resp_iov;
> struct page **pages;
> int tag;
>
> @@ -613,6 +614,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> sg = cmd->tvc_sgl;
> prot_sg = cmd->tvc_prot_sgl;
> pages = cmd->tvc_upages;
> + tvc_resp_iov = cmd->tvc_resp_iov;
> memset(cmd, 0, sizeof(*cmd));
> cmd->tvc_sgl = sg;
> cmd->tvc_prot_sgl = prot_sg;
> @@ -625,6 +627,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> cmd->tvc_data_direction = data_direction;
> cmd->tvc_nexus = tv_nexus;
> cmd->inflight = vhost_scsi_get_inflight(vq);
> + cmd->tvc_resp_iov = tvc_resp_iov;
>
> memcpy(cmd->tvc_cdb, cdb, VHOST_SCSI_MAX_CDB_SIZE);
>
> @@ -935,7 +938,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
> struct iov_iter in_iter, prot_iter, data_iter;
> u64 tag;
> u32 exp_data_len, data_direction;
> - int ret, prot_bytes, c = 0;
> + int ret, prot_bytes, i, c = 0;
> u16 lun;
> u8 task_attr;
> bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI);
> @@ -1092,7 +1095,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
> }
> cmd->tvc_vhost = vs;
> cmd->tvc_vq = vq;
> - cmd->tvc_resp_iov = vq->iov[vc.out];
> + for (i = 0; i < vc.in ; i++)
> + cmd->tvc_resp_iov[i] = vq->iov[vc.out + i];
Where is the guarantee that vc.in < UIO_MAXIOV?
> cmd->tvc_in_iovs = vc.in;
>
> pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
> @@ -1461,6 +1465,7 @@ static void vhost_scsi_destroy_vq_cmds(struct vhost_virtqueue *vq)
> kfree(tv_cmd->tvc_sgl);
> kfree(tv_cmd->tvc_prot_sgl);
> kfree(tv_cmd->tvc_upages);
> + kfree(tv_cmd->tvc_resp_iov);
> }
>
> sbitmap_free(&svq->scsi_tags);
> @@ -1508,6 +1513,14 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
> goto out;
> }
>
> + tv_cmd->tvc_resp_iov = kcalloc(UIO_MAXIOV,
> + sizeof(struct page *),
> + GFP_KERNEL);
Should sizeof(struct page *) be sizeof(struct iovec)?
> + if (!tv_cmd->tvc_resp_iov) {
> + pr_err("Unable to allocate tv_cmd->tvc_resp_iov\n");
> + goto out;
> + }
> +
> tv_cmd->tvc_prot_sgl = kcalloc(VHOST_SCSI_PREALLOC_PROT_SGLS,
> sizeof(struct scatterlist),
> GFP_KERNEL);
> --
> 2.25.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2023-01-11 19:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-11 6:07 [PATCH] vhost-scsi: unbreak any layout for response Jason Wang
2023-01-11 19:18 ` Stefan Hajnoczi [this message]
2023-01-12 4:02 ` Jason Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y78Lfmzr6s1BU3ri@fedora \
--to=stefanha@redhat.com \
--cc=bcodding@redhat.com \
--cc=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=nab@linux-iscsi.org \
--cc=netdev@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=viro@zeniv.linux.org.uk \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).