* [PATCH for 3.4] virtio-scsi: fix TMF use-after-free
@ 2012-04-18 13:46 Paolo Bonzini
2012-04-18 14:01 ` Zhi Yong Wu
2012-04-18 14:09 ` James Bottomley
0 siblings, 2 replies; 5+ messages in thread
From: Paolo Bonzini @ 2012-04-18 13:46 UTC (permalink / raw)
To: linux-kernel; +Cc: James Bottomley, linux-scsi, Hu Tao
Fix a race in TMF path, where cmd may have been already freed
by virtscsi_complete_free after waking up from the completion.
Cc: James Bottomley <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/scsi/virtio_scsi.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index efccd72..1b38431 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -175,7 +175,8 @@ static void virtscsi_complete_free(void *buf)
if (cmd->comp)
complete_all(cmd->comp);
- mempool_free(cmd, virtscsi_cmd_pool);
+ else
+ mempool_free(cmd, virtscsi_cmd_pool);
}
static void virtscsi_ctrl_done(struct virtqueue *vq)
@@ -311,21 +312,22 @@ out:
static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
{
DECLARE_COMPLETION_ONSTACK(comp);
- int ret;
+ int ret = FAILED;
cmd->comp = ∁
- ret = virtscsi_kick_cmd(vscsi, vscsi->ctrl_vq, cmd,
- sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
- GFP_NOIO);
- if (ret < 0)
- return FAILED;
+ if (virtscsi_kick_cmd(vscsi, vscsi->ctrl_vq, cmd,
+ sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
+ GFP_NOIO) < 0)
+ goto out;
wait_for_completion(&comp);
- if (cmd->resp.tmf.response != VIRTIO_SCSI_S_OK &&
- cmd->resp.tmf.response != VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
- return FAILED;
+ if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
+ cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
+ ret = SUCCESS;
- return SUCCESS;
+out:
+ mempool_free(cmd, virtscsi_cmd_pool);
+ return ret;
}
static int virtscsi_device_reset(struct scsi_cmnd *sc)
--
1.7.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for 3.4] virtio-scsi: fix TMF use-after-free
2012-04-18 13:46 [PATCH for 3.4] virtio-scsi: fix TMF use-after-free Paolo Bonzini
@ 2012-04-18 14:01 ` Zhi Yong Wu
2012-04-18 14:09 ` James Bottomley
1 sibling, 0 replies; 5+ messages in thread
From: Zhi Yong Wu @ 2012-04-18 14:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, James Bottomley, linux-scsi, Hu Tao
On Wed, Apr 18, 2012 at 9:46 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Fix a race in TMF path, where cmd may have been already freed
> by virtscsi_complete_free after waking up from the completion.
>
> Cc: James Bottomley <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> drivers/scsi/virtio_scsi.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index efccd72..1b38431 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -175,7 +175,8 @@ static void virtscsi_complete_free(void *buf)
>
> if (cmd->comp)
> complete_all(cmd->comp);
> - mempool_free(cmd, virtscsi_cmd_pool);
> + else
> + mempool_free(cmd, virtscsi_cmd_pool);
good catch
> }
>
> static void virtscsi_ctrl_done(struct virtqueue *vq)
> @@ -311,21 +312,22 @@ out:
> static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
> {
> DECLARE_COMPLETION_ONSTACK(comp);
> - int ret;
> + int ret = FAILED;
>
> cmd->comp = ∁
> - ret = virtscsi_kick_cmd(vscsi, vscsi->ctrl_vq, cmd,
> - sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
> - GFP_NOIO);
> - if (ret < 0)
> - return FAILED;
> + if (virtscsi_kick_cmd(vscsi, vscsi->ctrl_vq, cmd,
> + sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
> + GFP_NOIO) < 0)
> + goto out;
>
> wait_for_completion(&comp);
> - if (cmd->resp.tmf.response != VIRTIO_SCSI_S_OK &&
> - cmd->resp.tmf.response != VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
> - return FAILED;
> + if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
> + cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
> + ret = SUCCESS;
>
> - return SUCCESS;
> +out:
> + mempool_free(cmd, virtscsi_cmd_pool);
> + return ret;
> }
>
> static int virtscsi_device_reset(struct scsi_cmnd *sc)
> --
> 1.7.9.3
Reviewed-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Regards,
Zhi Yong Wu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 3.4] virtio-scsi: fix TMF use-after-free
2012-04-18 13:46 [PATCH for 3.4] virtio-scsi: fix TMF use-after-free Paolo Bonzini
2012-04-18 14:01 ` Zhi Yong Wu
@ 2012-04-18 14:09 ` James Bottomley
2012-05-01 8:09 ` Paolo Bonzini
1 sibling, 1 reply; 5+ messages in thread
From: James Bottomley @ 2012-04-18 14:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-scsi, Hu Tao
On Wed, 2012-04-18 at 15:46 +0200, Paolo Bonzini wrote:
> Fix a race in TMF path, where cmd may have been already freed
> by virtscsi_complete_free after waking up from the completion.
There's no may about this; the command will be freed long before the
completion waiter is awoken. The description could be clearer.
The problem is a use after free in virtscsi_tmf because the
virtio_scsi_command is freed before the completion returns.
The fix is to make callers specifying a completion responsible for
freeing the command in all cases.
James
> Cc: James Bottomley <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> drivers/scsi/virtio_scsi.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index efccd72..1b38431 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -175,7 +175,8 @@ static void virtscsi_complete_free(void *buf)
>
> if (cmd->comp)
> complete_all(cmd->comp);
> - mempool_free(cmd, virtscsi_cmd_pool);
> + else
> + mempool_free(cmd, virtscsi_cmd_pool);
> }
>
> static void virtscsi_ctrl_done(struct virtqueue *vq)
> @@ -311,21 +312,22 @@ out:
> static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
> {
> DECLARE_COMPLETION_ONSTACK(comp);
> - int ret;
> + int ret = FAILED;
>
> cmd->comp = ∁
> - ret = virtscsi_kick_cmd(vscsi, vscsi->ctrl_vq, cmd,
> - sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
> - GFP_NOIO);
> - if (ret < 0)
> - return FAILED;
> + if (virtscsi_kick_cmd(vscsi, vscsi->ctrl_vq, cmd,
> + sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
> + GFP_NOIO) < 0)
> + goto out;
>
> wait_for_completion(&comp);
> - if (cmd->resp.tmf.response != VIRTIO_SCSI_S_OK &&
> - cmd->resp.tmf.response != VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
> - return FAILED;
> + if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
> + cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
> + ret = SUCCESS;
>
> - return SUCCESS;
> +out:
> + mempool_free(cmd, virtscsi_cmd_pool);
> + return ret;
> }
>
> static int virtscsi_device_reset(struct scsi_cmnd *sc)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 3.4] virtio-scsi: fix TMF use-after-free
2012-04-18 14:09 ` James Bottomley
@ 2012-05-01 8:09 ` Paolo Bonzini
2012-05-01 8:52 ` James Bottomley
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2012-05-01 8:09 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-kernel, linux-scsi, Hu Tao
Il 18/04/2012 16:09, James Bottomley ha scritto:
>> > Fix a race in TMF path, where cmd may have been already freed
>> > by virtscsi_complete_free after waking up from the completion.
> There's no may about this; the command will be freed long before the
> completion waiter is awoken. The description could be clearer.
>
> The problem is a use after free in virtscsi_tmf because the
> virtio_scsi_command is freed before the completion returns.
>
> The fix is to make callers specifying a completion responsible for
> freeing the command in all cases.
I don't see this in the pull request, were you waiting for v2 with a
better commit message?
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 3.4] virtio-scsi: fix TMF use-after-free
2012-05-01 8:09 ` Paolo Bonzini
@ 2012-05-01 8:52 ` James Bottomley
0 siblings, 0 replies; 5+ messages in thread
From: James Bottomley @ 2012-05-01 8:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-scsi, Hu Tao
On Tue, 2012-05-01 at 10:09 +0200, Paolo Bonzini wrote:
> Il 18/04/2012 16:09, James Bottomley ha scritto:
> >> > Fix a race in TMF path, where cmd may have been already freed
> >> > by virtscsi_complete_free after waking up from the completion.
> > There's no may about this; the command will be freed long before the
> > completion waiter is awoken. The description could be clearer.
> >
> > The problem is a use after free in virtscsi_tmf because the
> > virtio_scsi_command is freed before the completion returns.
> >
> > The fix is to make callers specifying a completion responsible for
> > freeing the command in all cases.
>
> I don't see this in the pull request, were you waiting for v2 with a
> better commit message?
That would be because you didn't reply. I was expecting either a
comment or a rewording of the change log. My inbox works by threads
rising to the top and completed ones going into the patch queue. If
no-one replys to a thread expecting one, it just gets lost.
James
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-01 8:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-18 13:46 [PATCH for 3.4] virtio-scsi: fix TMF use-after-free Paolo Bonzini
2012-04-18 14:01 ` Zhi Yong Wu
2012-04-18 14:09 ` James Bottomley
2012-05-01 8:09 ` Paolo Bonzini
2012-05-01 8:52 ` James Bottomley
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).