Commit 6dd88fd5 authored by Jason Wang's avatar Jason Wang Committed by Michael S. Tsirkin

vhost-scsi: unbreak any layout for response

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
79c14141 ("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
limitation that we advertise to the vhost_get_vq_desc().

Testing with the hacked virtio-scsi driver that use 1 descriptor for 1
byte in the response.
Reported-by: default avatarAl Viro <viro@zeniv.linux.org.uk>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Fixes: a77ec83a ("vhost/scsi: fix reuse of &vq->iov[out] in response")
Signed-off-by: default avatarJason Wang <jasowang@redhat.com>
Message-Id: <20230119073647.76467-1-jasowang@redhat.com>
Signed-off-by: default avatarMichael S. Tsirkin <mst@redhat.com>
Reviewed-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
parent 3f7b75ab
...@@ -80,7 +80,7 @@ struct vhost_scsi_cmd { ...@@ -80,7 +80,7 @@ struct vhost_scsi_cmd {
struct scatterlist *tvc_prot_sgl; struct scatterlist *tvc_prot_sgl;
struct page **tvc_upages; struct page **tvc_upages;
/* Pointer to response header iovec */ /* Pointer to response header iovec */
struct iovec tvc_resp_iov; struct iovec *tvc_resp_iov;
/* Pointer to vhost_scsi for our device */ /* Pointer to vhost_scsi for our device */
struct vhost_scsi *tvc_vhost; struct vhost_scsi *tvc_vhost;
/* Pointer to vhost_virtqueue for the cmd */ /* Pointer to vhost_virtqueue for the cmd */
...@@ -563,7 +563,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work) ...@@ -563,7 +563,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
memcpy(v_rsp.sense, cmd->tvc_sense_buf, memcpy(v_rsp.sense, cmd->tvc_sense_buf,
se_cmd->scsi_sense_length); 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)); cmd->tvc_in_iovs, sizeof(v_rsp));
ret = copy_to_iter(&v_rsp, sizeof(v_rsp), &iov_iter); ret = copy_to_iter(&v_rsp, sizeof(v_rsp), &iov_iter);
if (likely(ret == sizeof(v_rsp))) { if (likely(ret == sizeof(v_rsp))) {
...@@ -594,6 +594,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg, ...@@ -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_cmd *cmd;
struct vhost_scsi_nexus *tv_nexus; struct vhost_scsi_nexus *tv_nexus;
struct scatterlist *sg, *prot_sg; struct scatterlist *sg, *prot_sg;
struct iovec *tvc_resp_iov;
struct page **pages; struct page **pages;
int tag; int tag;
...@@ -613,6 +614,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg, ...@@ -613,6 +614,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
sg = cmd->tvc_sgl; sg = cmd->tvc_sgl;
prot_sg = cmd->tvc_prot_sgl; prot_sg = cmd->tvc_prot_sgl;
pages = cmd->tvc_upages; pages = cmd->tvc_upages;
tvc_resp_iov = cmd->tvc_resp_iov;
memset(cmd, 0, sizeof(*cmd)); memset(cmd, 0, sizeof(*cmd));
cmd->tvc_sgl = sg; cmd->tvc_sgl = sg;
cmd->tvc_prot_sgl = prot_sg; cmd->tvc_prot_sgl = prot_sg;
...@@ -625,6 +627,7 @@ vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg, ...@@ -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_data_direction = data_direction;
cmd->tvc_nexus = tv_nexus; cmd->tvc_nexus = tv_nexus;
cmd->inflight = vhost_scsi_get_inflight(vq); cmd->inflight = vhost_scsi_get_inflight(vq);
cmd->tvc_resp_iov = tvc_resp_iov;
memcpy(cmd->tvc_cdb, cdb, VHOST_SCSI_MAX_CDB_SIZE); 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) ...@@ -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; struct iov_iter in_iter, prot_iter, data_iter;
u64 tag; u64 tag;
u32 exp_data_len, data_direction; u32 exp_data_len, data_direction;
int ret, prot_bytes, c = 0; int ret, prot_bytes, i, c = 0;
u16 lun; u16 lun;
u8 task_attr; u8 task_attr;
bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI); 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) ...@@ -1092,7 +1095,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
} }
cmd->tvc_vhost = vs; cmd->tvc_vhost = vs;
cmd->tvc_vq = vq; 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];
cmd->tvc_in_iovs = vc.in; cmd->tvc_in_iovs = vc.in;
pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n", 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) ...@@ -1461,6 +1465,7 @@ static void vhost_scsi_destroy_vq_cmds(struct vhost_virtqueue *vq)
kfree(tv_cmd->tvc_sgl); kfree(tv_cmd->tvc_sgl);
kfree(tv_cmd->tvc_prot_sgl); kfree(tv_cmd->tvc_prot_sgl);
kfree(tv_cmd->tvc_upages); kfree(tv_cmd->tvc_upages);
kfree(tv_cmd->tvc_resp_iov);
} }
sbitmap_free(&svq->scsi_tags); sbitmap_free(&svq->scsi_tags);
...@@ -1508,6 +1513,14 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds) ...@@ -1508,6 +1513,14 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
goto out; goto out;
} }
tv_cmd->tvc_resp_iov = kcalloc(UIO_MAXIOV,
sizeof(struct iovec),
GFP_KERNEL);
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, tv_cmd->tvc_prot_sgl = kcalloc(VHOST_SCSI_PREALLOC_PROT_SGLS,
sizeof(struct scatterlist), sizeof(struct scatterlist),
GFP_KERNEL); GFP_KERNEL);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment