Re: [PATCH 2/2] virtio-balloon: correct used length
On Fri, Nov 26, 2021 at 3:37 PM Michael S. Tsirkin wrote:
>
> On Fri, Nov 26, 2021 at 10:45:43AM +0800, Jason Wang wrote:
> > On Fri, Nov 26, 2021 at 12:14 AM Michael S. Tsirkin wrote:
> > >
> > > On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> > > > Spec said:
> > > >
> > > > "and len the total of bytes written into the buffer."
> > > >
> > > > For inflateq, deflateq and statsq, we don't process in_sg so the used
> > > > length should be zero. For free_page_vq, since the pages could be
> > > > changed in the destination, we should make all pages used for safety.
> > >
> > > Yea, about that, I know I said it, but I was wrong, sorry.
> > >
> > > Spec says this:
> > >
> > > \field{len} is particularly useful
> > > for drivers using untrusted buffers: if a driver does not know
> > > exactly
> > > how much has been written by the device, the driver would have to
> > > zero
> > > the buffer in advance to ensure no data leakage occurs.
> > >
> > > For example, a network driver may hand a received buffer directly
> > > to
> > > an unprivileged userspace application. If the network device has
> > > not
> > > overwritten the bytes which were in that buffer, this could leak
> > > the
> > > contents of freed memory from other processes to the application.
> > >
> > >
> > > In other words, device must guarantee that used length was
> > > written into. Since we don't know that, we really should
> > > write 0 there, and the fact we don't is a spec violation.
> >
> > The problem is, if we write 0, the driver may assume there's no change
> > on those pages?
> >
> > Thanks
>
> No:
>
>
> The driver MUST NOT make assumptions about data in device-writable buffers
> beyond the first \field{len} bytes, and SHOULD ignore this data.
Good to know this. Will fix it in V2.
Thanks
>
>
>
> > >
> > >
> > > > Signed-off-by: Jason Wang
> > > > ---
> > > > hw/virtio/virtio-balloon.c | 8 +---
> > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > > index 17de2558cb..fb4426ac0c 100644
> > > > --- a/hw/virtio/virtio-balloon.c
> > > > +++ b/hw/virtio/virtio-balloon.c
> > > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> > > > return;
> > > > }
> > > >
> > > > -virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > > > +virtqueue_push(s->svq, s->stats_vq_elem, 0);
> > > > virtio_notify(vdev, s->svq);
> > > > g_free(s->stats_vq_elem);
> > > > s->stats_vq_elem = NULL;
> > > > @@ -438,7 +438,7 @@ static void
> > > > virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > > > memory_region_unref(section.mr);
> > > > }
> > > >
> > > > -virtqueue_push(vq, elem, offset);
> > > > +virtqueue_push(vq, elem, 0);
> > > > virtio_notify(vdev, vq);
> > > > g_free(elem);
> > > > virtio_balloon_pbp_free();
> > > > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > > > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > > VirtQueue *vq = dev->free_page_vq;
> > > > bool ret = true;
> > > > +size_t used = 0;
> > > > int i;
> > > >
> > > > while (dev->block_iothread) {
> > > > @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon
> > > > *dev)
> > > > for (i = 0; i < elem->in_num; i++) {
> > > > qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > > >elem->in_sg[i].iov_len);
> > > > +used += elem->in_sg[i].iov_len;
> > > > }
> > > > }
> > > >
> > > > out:
> > > > -virtqueue_push(vq, elem, 1);
> > > > +virtqueue_push(vq, elem, used);
> > > > g_free(elem);
> > > > return ret;
> > > > }
> > > > --
> > > > 2.25.1
> > >
>
Re: [PATCH 2/2] virtio-balloon: correct used length
On Fri, Nov 26, 2021 at 10:45:43AM +0800, Jason Wang wrote:
> On Fri, Nov 26, 2021 at 12:14 AM Michael S. Tsirkin wrote:
> >
> > On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> > > Spec said:
> > >
> > > "and len the total of bytes written into the buffer."
> > >
> > > For inflateq, deflateq and statsq, we don't process in_sg so the used
> > > length should be zero. For free_page_vq, since the pages could be
> > > changed in the destination, we should make all pages used for safety.
> >
> > Yea, about that, I know I said it, but I was wrong, sorry.
> >
> > Spec says this:
> >
> > \field{len} is particularly useful
> > for drivers using untrusted buffers: if a driver does not know
> > exactly
> > how much has been written by the device, the driver would have to
> > zero
> > the buffer in advance to ensure no data leakage occurs.
> >
> > For example, a network driver may hand a received buffer directly to
> > an unprivileged userspace application. If the network device has
> > not
> > overwritten the bytes which were in that buffer, this could leak the
> > contents of freed memory from other processes to the application.
> >
> >
> > In other words, device must guarantee that used length was
> > written into. Since we don't know that, we really should
> > write 0 there, and the fact we don't is a spec violation.
>
> The problem is, if we write 0, the driver may assume there's no change
> on those pages?
>
> Thanks
No:
The driver MUST NOT make assumptions about data in device-writable buffers
beyond the first \field{len} bytes, and SHOULD ignore this data.
> >
> >
> > > Signed-off-by: Jason Wang
> > > ---
> > > hw/virtio/virtio-balloon.c | 8 +---
> > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > index 17de2558cb..fb4426ac0c 100644
> > > --- a/hw/virtio/virtio-balloon.c
> > > +++ b/hw/virtio/virtio-balloon.c
> > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> > > return;
> > > }
> > >
> > > -virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > > +virtqueue_push(s->svq, s->stats_vq_elem, 0);
> > > virtio_notify(vdev, s->svq);
> > > g_free(s->stats_vq_elem);
> > > s->stats_vq_elem = NULL;
> > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice
> > > *vdev, VirtQueue *vq)
> > > memory_region_unref(section.mr);
> > > }
> > >
> > > -virtqueue_push(vq, elem, offset);
> > > +virtqueue_push(vq, elem, 0);
> > > virtio_notify(vdev, vq);
> > > g_free(elem);
> > > virtio_balloon_pbp_free();
> > > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > VirtQueue *vq = dev->free_page_vq;
> > > bool ret = true;
> > > +size_t used = 0;
> > > int i;
> > >
> > > while (dev->block_iothread) {
> > > @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > > for (i = 0; i < elem->in_num; i++) {
> > > qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > >elem->in_sg[i].iov_len);
> > > +used += elem->in_sg[i].iov_len;
> > > }
> > > }
> > >
> > > out:
> > > -virtqueue_push(vq, elem, 1);
> > > +virtqueue_push(vq, elem, used);
> > > g_free(elem);
> > > return ret;
> > > }
> > > --
> > > 2.25.1
> >
Re: [PATCH 2/2] virtio-balloon: correct used length
On Fri, Nov 26, 2021 at 12:14 AM Michael S. Tsirkin wrote:
>
> On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> > Spec said:
> >
> > "and len the total of bytes written into the buffer."
> >
> > For inflateq, deflateq and statsq, we don't process in_sg so the used
> > length should be zero. For free_page_vq, since the pages could be
> > changed in the destination, we should make all pages used for safety.
>
> Yea, about that, I know I said it, but I was wrong, sorry.
>
> Spec says this:
>
> \field{len} is particularly useful
> for drivers using untrusted buffers: if a driver does not know exactly
> how much has been written by the device, the driver would have to zero
> the buffer in advance to ensure no data leakage occurs.
>
> For example, a network driver may hand a received buffer directly to
> an unprivileged userspace application. If the network device has not
> overwritten the bytes which were in that buffer, this could leak the
> contents of freed memory from other processes to the application.
>
>
> In other words, device must guarantee that used length was
> written into. Since we don't know that, we really should
> write 0 there, and the fact we don't is a spec violation.
The problem is, if we write 0, the driver may assume there's no change
on those pages?
Thanks
>
>
> > Signed-off-by: Jason Wang
> > ---
> > hw/virtio/virtio-balloon.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 17de2558cb..fb4426ac0c 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> > return;
> > }
> >
> > -virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > +virtqueue_push(s->svq, s->stats_vq_elem, 0);
> > virtio_notify(vdev, s->svq);
> > g_free(s->stats_vq_elem);
> > s->stats_vq_elem = NULL;
> > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice
> > *vdev, VirtQueue *vq)
> > memory_region_unref(section.mr);
> > }
> >
> > -virtqueue_push(vq, elem, offset);
> > +virtqueue_push(vq, elem, 0);
> > virtio_notify(vdev, vq);
> > g_free(elem);
> > virtio_balloon_pbp_free();
> > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > VirtQueue *vq = dev->free_page_vq;
> > bool ret = true;
> > +size_t used = 0;
> > int i;
> >
> > while (dev->block_iothread) {
> > @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > for (i = 0; i < elem->in_num; i++) {
> > qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> >elem->in_sg[i].iov_len);
> > +used += elem->in_sg[i].iov_len;
> > }
> > }
> >
> > out:
> > -virtqueue_push(vq, elem, 1);
> > +virtqueue_push(vq, elem, used);
> > g_free(elem);
> > return ret;
> > }
> > --
> > 2.25.1
>
Re: [PATCH 2/2] virtio-balloon: correct used length
On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> Spec said:
>
> "and len the total of bytes written into the buffer."
>
> For inflateq, deflateq and statsq, we don't process in_sg so the used
> length should be zero. For free_page_vq, since the pages could be
> changed in the destination, we should make all pages used for safety.
Yea, about that, I know I said it, but I was wrong, sorry.
Spec says this:
\field{len} is particularly useful
for drivers using untrusted buffers: if a driver does not know exactly
how much has been written by the device, the driver would have to zero
the buffer in advance to ensure no data leakage occurs.
For example, a network driver may hand a received buffer directly to
an unprivileged userspace application. If the network device has not
overwritten the bytes which were in that buffer, this could leak the
contents of freed memory from other processes to the application.
In other words, device must guarantee that used length was
written into. Since we don't know that, we really should
write 0 there, and the fact we don't is a spec violation.
> Signed-off-by: Jason Wang
> ---
> hw/virtio/virtio-balloon.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 17de2558cb..fb4426ac0c 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> return;
> }
>
> -virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> +virtqueue_push(s->svq, s->stats_vq_elem, 0);
> virtio_notify(vdev, s->svq);
> g_free(s->stats_vq_elem);
> s->stats_vq_elem = NULL;
> @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice
> *vdev, VirtQueue *vq)
> memory_region_unref(section.mr);
> }
>
> -virtqueue_push(vq, elem, offset);
> +virtqueue_push(vq, elem, 0);
> virtio_notify(vdev, vq);
> g_free(elem);
> virtio_balloon_pbp_free();
> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtQueue *vq = dev->free_page_vq;
> bool ret = true;
> +size_t used = 0;
> int i;
>
> while (dev->block_iothread) {
> @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> for (i = 0; i < elem->in_num; i++) {
> qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
>elem->in_sg[i].iov_len);
> +used += elem->in_sg[i].iov_len;
> }
> }
>
> out:
> -virtqueue_push(vq, elem, 1);
> +virtqueue_push(vq, elem, used);
> g_free(elem);
> return ret;
> }
> --
> 2.25.1
[PATCH 2/2] virtio-balloon: correct used length
Spec said:
"and len the total of bytes written into the buffer."
For inflateq, deflateq and statsq, we don't process in_sg so the used
length should be zero. For free_page_vq, since the pages could be
changed in the destination, we should make all pages used for safety.
Signed-off-by: Jason Wang
---
hw/virtio/virtio-balloon.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 17de2558cb..fb4426ac0c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
return;
}
-virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
+virtqueue_push(s->svq, s->stats_vq_elem, 0);
virtio_notify(vdev, s->svq);
g_free(s->stats_vq_elem);
s->stats_vq_elem = NULL;
@@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice
*vdev, VirtQueue *vq)
memory_region_unref(section.mr);
}
-virtqueue_push(vq, elem, offset);
+virtqueue_push(vq, elem, 0);
virtio_notify(vdev, vq);
g_free(elem);
virtio_balloon_pbp_free();
@@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtQueue *vq = dev->free_page_vq;
bool ret = true;
+size_t used = 0;
int i;
while (dev->block_iothread) {
@@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
for (i = 0; i < elem->in_num; i++) {
qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
elem->in_sg[i].iov_len);
+used += elem->in_sg[i].iov_len;
}
}
out:
-virtqueue_push(vq, elem, 1);
+virtqueue_push(vq, elem, used);
g_free(elem);
return ret;
}
--
2.25.1
