Christoph Hellwig
2017-Jan-04 05:25 UTC
4.10 and -stable fix for virtio_blk and virtually mapped stacks
Without this fix attempts to do scsi passthrough on virtio_blk will crash the system on virtually mapped stacks, which is something happening during boot with many distros.
Christoph Hellwig
2017-Jan-04 05:25 UTC
[PATCH] virtio_blk: avoid DMA to stack for the sense buffer
Most users of BLOCK_PC requests allocate the sense buffer on the stack,
so to avoid DMA to the stack copy them to a field in the heap allocated
virtblk_req structure. Without that any attempt at SCSI passthrough I/O,
including the SG_IO ioctl from userspace will crash the kernel. Note that
this includes running tools like hdparm even when the host does not have
SCSI passthrough enabled.
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
drivers/block/virtio_blk.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 5545a67..3c3b8f6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -56,6 +56,7 @@ struct virtblk_req {
struct virtio_blk_outhdr out_hdr;
struct virtio_scsi_inhdr in_hdr;
u8 status;
+ u8 sense[SCSI_SENSE_BUFFERSIZE];
struct scatterlist sg[];
};
@@ -102,7 +103,8 @@ static int __virtblk_add_req(struct virtqueue *vq,
}
if (type == cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_SCSI_CMD)) {
- sg_init_one(&sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE);
+ memcpy(vbr->sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE);
+ sg_init_one(&sense, vbr->sense, SCSI_SENSE_BUFFERSIZE);
sgs[num_out + num_in++] = &sense;
sg_init_one(&inhdr, &vbr->in_hdr, sizeof(vbr->in_hdr));
sgs[num_out + num_in++] = &inhdr;
--
2.1.4
Jason Wang
2017-Jan-04 07:44 UTC
[PATCH] virtio_blk: avoid DMA to stack for the sense buffer
On 2017?01?04? 13:25, Christoph Hellwig wrote:> Most users of BLOCK_PC requests allocate the sense buffer on the stack, > so to avoid DMA to the stack copy them to a field in the heap allocated > virtblk_req structure. Without that any attempt at SCSI passthrough I/O, > including the SG_IO ioctl from userspace will crash the kernel. Note that > this includes running tools like hdparm even when the host does not have > SCSI passthrough enabled. > > Signed-off-by: Christoph Hellwig <hch at lst.de> > --- > drivers/block/virtio_blk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 5545a67..3c3b8f6 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -56,6 +56,7 @@ struct virtblk_req { > struct virtio_blk_outhdr out_hdr; > struct virtio_scsi_inhdr in_hdr; > u8 status; > + u8 sense[SCSI_SENSE_BUFFERSIZE]; > struct scatterlist sg[]; > }; > > @@ -102,7 +103,8 @@ static int __virtblk_add_req(struct virtqueue *vq, > } > > if (type == cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_SCSI_CMD)) { > - sg_init_one(&sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE); > + memcpy(vbr->sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE); > + sg_init_one(&sense, vbr->sense, SCSI_SENSE_BUFFERSIZE); > sgs[num_out + num_in++] = &sense; > sg_init_one(&inhdr, &vbr->in_hdr, sizeof(vbr->in_hdr)); > sgs[num_out + num_in++] = &inhdr;Acked-by: Jason Wang <jasowang at redhat.com>
Hi Christoph, 2017-01-04 6:25 GMT+01:00 Christoph Hellwig <hch at lst.de>:> Most users of BLOCK_PC requests allocate the sense buffer on the stack, > so to avoid DMA to the stack copy them to a field in the heap allocated > virtblk_req structure. Without that any attempt at SCSI passthrough I/O, > including the SG_IO ioctl from userspace will crash the kernel. Note that > this includes running tools like hdparm even when the host does not have > SCSI passthrough enabled.This sounds scary. Could you share how to reproduce it, this should go into stable if it's the case. Thanks, Jinpu> > Signed-off-by: Christoph Hellwig <hch at lst.de> > --- > drivers/block/virtio_blk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 5545a67..3c3b8f6 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -56,6 +56,7 @@ struct virtblk_req { > struct virtio_blk_outhdr out_hdr; > struct virtio_scsi_inhdr in_hdr; > u8 status; > + u8 sense[SCSI_SENSE_BUFFERSIZE]; > struct scatterlist sg[]; > }; > > @@ -102,7 +103,8 @@ static int __virtblk_add_req(struct virtqueue *vq, > } > > if (type == cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_SCSI_CMD)) { > - sg_init_one(&sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE); > + memcpy(vbr->sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE); > + sg_init_one(&sense, vbr->sense, SCSI_SENSE_BUFFERSIZE); > sgs[num_out + num_in++] = &sense; > sg_init_one(&inhdr, &vbr->in_hdr, sizeof(vbr->in_hdr)); > sgs[num_out + num_in++] = &inhdr; > -- > 2.1.4 >
Michael S. Tsirkin
2017-Jan-09 16:33 UTC
[PATCH] virtio_blk: avoid DMA to stack for the sense buffer
On Wed, Jan 04, 2017 at 08:25:05AM +0300, Christoph Hellwig wrote:> Most users of BLOCK_PC requests allocate the sense buffer on the stack, > so to avoid DMA to the stack copy them to a field in the heap allocated > virtblk_req structure. Without that any attempt at SCSI passthrough I/O, > including the SG_IO ioctl from userspace will crash the kernel. Note that > this includes running tools like hdparm even when the host does not have > SCSI passthrough enabled. > > Signed-off-by: Christoph Hellwig <hch at lst.de> > --- > drivers/block/virtio_blk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 5545a67..3c3b8f6 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -56,6 +56,7 @@ struct virtblk_req { > struct virtio_blk_outhdr out_hdr; > struct virtio_scsi_inhdr in_hdr; > u8 status; > + u8 sense[SCSI_SENSE_BUFFERSIZE]; > struct scatterlist sg[]; > }; > > @@ -102,7 +103,8 @@ static int __virtblk_add_req(struct virtqueue *vq, > } > > if (type == cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_SCSI_CMD)) { > - sg_init_one(&sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE); > + memcpy(vbr->sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE); > + sg_init_one(&sense, vbr->sense, SCSI_SENSE_BUFFERSIZE);I would prefer sizeof(vbr->sense) here to avoid duplication. Otherwise: Acked-by: Michael S. Tsirkin <mst at redhat.com>> sgs[num_out + num_in++] = &sense; > sg_init_one(&inhdr, &vbr->in_hdr, sizeof(vbr->in_hdr)); > sgs[num_out + num_in++] = &inhdr; > -- > 2.1.4