Vincent Whitchurch
2021-Nov-11 16:04 UTC
[PATCH v2 1/2] i2c: virtio: disable timeout handling
If a timeout is hit, it can result is incorrect data on the I2C bus
and/or memory corruptions in the guest since the device can still be
operating on the buffers it was given while the guest has freed them.
Here is, for example, the start of a slub_debug splat which was
triggered on the next transfer after one transfer was forced to timeout
by setting a breakpoint in the backend (rust-vmm/vhost-device):
BUG kmalloc-1k (Not tainted): Poison overwritten
First byte 0x1 instead of 0x6b
Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
__kmalloc+0xc2/0x1c9
virtio_i2c_xfer+0x65/0x35c
__i2c_transfer+0x429/0x57d
i2c_transfer+0x115/0x134
i2cdev_ioctl_rdwr+0x16a/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
kfree+0x1bd/0x1cc
virtio_i2c_xfer+0x32e/0x35c
__i2c_transfer+0x429/0x57d
i2c_transfer+0x115/0x134
i2cdev_ioctl_rdwr+0x16a/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
There is no simple fix for this (the driver would have to always create
bounce buffers and hold on to them until the device eventually returns
the buffers), so just disable the timeout support for now.
Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend
driver")
Acked-by: Jie Deng <jie.deng at intel.com>
Signed-off-by: Vincent Whitchurch <vincent.whitchurch at axis.com>
---
drivers/i2c/busses/i2c-virtio.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index f10a603b13fb..7b2474e6876f 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
static int virtio_i2c_complete_reqs(struct virtqueue *vq,
struct virtio_i2c_req *reqs,
- struct i2c_msg *msgs, int num,
- bool timedout)
+ struct i2c_msg *msgs, int num)
{
struct virtio_i2c_req *req;
- bool failed = timedout;
+ bool failed = false;
unsigned int len;
int i, j = 0;
@@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
j++;
}
- return timedout ? -ETIMEDOUT : j;
+ return j;
}
static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
@@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs,
struct virtio_i2c *vi = i2c_get_adapdata(adap);
struct virtqueue *vq = vi->vq;
struct virtio_i2c_req *reqs;
- unsigned long time_left;
int count;
reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
@@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs,
reinit_completion(&vi->completion);
virtqueue_kick(vq);
- time_left = wait_for_completion_timeout(&vi->completion,
adap->timeout);
- if (!time_left)
- dev_err(&adap->dev, "virtio i2c backend timeout.\n");
+ wait_for_completion(&vi->completion);
- count = virtio_i2c_complete_reqs(vq, reqs, msgs, count, !time_left);
+ count = virtio_i2c_complete_reqs(vq, reqs, msgs, count);
err_free:
kfree(reqs);
--
2.28.0
Michael S. Tsirkin
2021-Nov-11 16:46 UTC
[PATCH v2 1/2] i2c: virtio: disable timeout handling
On Thu, Nov 11, 2021 at 05:04:11PM +0100, Vincent Whitchurch wrote:> If a timeout is hit, it can result is incorrect data on the I2C bus > and/or memory corruptions in the guest since the device can still be > operating on the buffers it was given while the guest has freed them. > > Here is, for example, the start of a slub_debug splat which was > triggered on the next transfer after one transfer was forced to timeout > by setting a breakpoint in the backend (rust-vmm/vhost-device): > > BUG kmalloc-1k (Not tainted): Poison overwritten > First byte 0x1 instead of 0x6b > Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29 > __kmalloc+0xc2/0x1c9 > virtio_i2c_xfer+0x65/0x35c > __i2c_transfer+0x429/0x57d > i2c_transfer+0x115/0x134 > i2cdev_ioctl_rdwr+0x16a/0x1de > i2cdev_ioctl+0x247/0x2ed > vfs_ioctl+0x21/0x30 > sys_ioctl+0xb18/0xb41 > Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29 > kfree+0x1bd/0x1cc > virtio_i2c_xfer+0x32e/0x35c > __i2c_transfer+0x429/0x57d > i2c_transfer+0x115/0x134 > i2cdev_ioctl_rdwr+0x16a/0x1de > i2cdev_ioctl+0x247/0x2ed > vfs_ioctl+0x21/0x30 > sys_ioctl+0xb18/0xb41 > > There is no simple fix for this (the driver would have to always create > bounce buffers and hold on to them until the device eventually returns > the buffers), so just disable the timeout support for now. > > Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver") > Acked-by: Jie Deng <jie.deng at intel.com> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch at axis.com>Acked-by: Michael S. Tsirkin <mst at redhat.com> For an eventual fix, I think you'd have to reset the device, then you can get free up the outstanding buffers. This has to be done carefully to make sure it does not race with interrupts and/or new requests, typically not easy.> --- > drivers/i2c/busses/i2c-virtio.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c > index f10a603b13fb..7b2474e6876f 100644 > --- a/drivers/i2c/busses/i2c-virtio.c > +++ b/drivers/i2c/busses/i2c-virtio.c > @@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq, > > static int virtio_i2c_complete_reqs(struct virtqueue *vq, > struct virtio_i2c_req *reqs, > - struct i2c_msg *msgs, int num, > - bool timedout) > + struct i2c_msg *msgs, int num) > { > struct virtio_i2c_req *req; > - bool failed = timedout; > + bool failed = false; > unsigned int len; > int i, j = 0; > > @@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq, > j++; > } > > - return timedout ? -ETIMEDOUT : j; > + return j; > } > > static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > struct virtio_i2c *vi = i2c_get_adapdata(adap); > struct virtqueue *vq = vi->vq; > struct virtio_i2c_req *reqs; > - unsigned long time_left; > int count; > > reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL); > @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > reinit_completion(&vi->completion); > virtqueue_kick(vq); > > - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout); > - if (!time_left) > - dev_err(&adap->dev, "virtio i2c backend timeout.\n"); > + wait_for_completion(&vi->completion); > > - count = virtio_i2c_complete_reqs(vq, reqs, msgs, count, !time_left); > + count = virtio_i2c_complete_reqs(vq, reqs, msgs, count); > > err_free: > kfree(reqs); > -- > 2.28.0
On 11-11-21, 17:04, Vincent Whitchurch wrote:> If a timeout is hit, it can result is incorrect data on the I2C bus > and/or memory corruptions in the guest since the device can still be > operating on the buffers it was given while the guest has freed them. > > Here is, for example, the start of a slub_debug splat which was > triggered on the next transfer after one transfer was forced to timeout > by setting a breakpoint in the backend (rust-vmm/vhost-device): > > BUG kmalloc-1k (Not tainted): Poison overwritten > First byte 0x1 instead of 0x6b > Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29 > __kmalloc+0xc2/0x1c9 > virtio_i2c_xfer+0x65/0x35c > __i2c_transfer+0x429/0x57d > i2c_transfer+0x115/0x134 > i2cdev_ioctl_rdwr+0x16a/0x1de > i2cdev_ioctl+0x247/0x2ed > vfs_ioctl+0x21/0x30 > sys_ioctl+0xb18/0xb41 > Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29 > kfree+0x1bd/0x1cc > virtio_i2c_xfer+0x32e/0x35c > __i2c_transfer+0x429/0x57d > i2c_transfer+0x115/0x134 > i2cdev_ioctl_rdwr+0x16a/0x1de > i2cdev_ioctl+0x247/0x2ed > vfs_ioctl+0x21/0x30 > sys_ioctl+0xb18/0xb41 > > There is no simple fix for this (the driver would have to always create > bounce buffers and hold on to them until the device eventually returns > the buffers), so just disable the timeout support for now. > > Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver") > Acked-by: Jie Deng <jie.deng at intel.com> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch at axis.com> > --- > drivers/i2c/busses/i2c-virtio.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c > index f10a603b13fb..7b2474e6876f 100644 > --- a/drivers/i2c/busses/i2c-virtio.c > +++ b/drivers/i2c/busses/i2c-virtio.c > @@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq, > > static int virtio_i2c_complete_reqs(struct virtqueue *vq, > struct virtio_i2c_req *reqs, > - struct i2c_msg *msgs, int num, > - bool timedout) > + struct i2c_msg *msgs, int num) > { > struct virtio_i2c_req *req; > - bool failed = timedout; > + bool failed = false; > unsigned int len; > int i, j = 0; > > @@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq, > j++; > } > > - return timedout ? -ETIMEDOUT : j; > + return j; > } > > static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > struct virtio_i2c *vi = i2c_get_adapdata(adap); > struct virtqueue *vq = vi->vq; > struct virtio_i2c_req *reqs; > - unsigned long time_left; > int count; > > reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL); > @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > reinit_completion(&vi->completion); > virtqueue_kick(vq); > > - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout); > - if (!time_left) > - dev_err(&adap->dev, "virtio i2c backend timeout.\n"); > + wait_for_completion(&vi->completion);I thought we decided on making this in insanely high value instead ? -- viresh