diff mbox series

vhost-scsi: unbreak any layout for response

Message ID 20230111060730.24779-1-jasowang@redhat.com (mailing list archive)
State New, archived
Headers show
Series vhost-scsi: unbreak any layout for response | expand

Commit Message

Jason Wang Jan. 11, 2023, 6:07 a.m. UTC
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(-)

Comments

Stefan Hajnoczi Jan. 11, 2023, 7:18 p.m. UTC | #1
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
>
Jason Wang Jan. 12, 2023, 4:02 a.m. UTC | #2
On Thu, Jan 12, 2023 at 3:18 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> 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?
>

We limit it here in the vhost_virtqueue structure:

        struct iovec iov[UIO_MAXIOV];

And we pass sizeof(vq->iov) to vhost_get_vq_desc():

        vc->head = vhost_get_vq_desc(vq, vq->iov,
                                     ARRAY_SIZE(vq->iov), &vc->out, &vc->in,
                                     NULL, NULL);

> >               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)?

Yes, I will fix it.

Thanks

>
> > +             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
> >
diff mbox series

Patch

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];
 		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);
+		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);