[PATCH] virtio_console: do not free control-out buffers on remove

[PATCH] virtio_console: do not free control-out buffers on remove

[Jia Jia]

__send_control_msg() publishes &portdev->cpkt as the control-out
virtqueue cookie. remove_vqs() walks every virtqueue and passes leftover
cookies to free_buf(), which treats them as struct port_buffer and
reads sgpages.

If a control message is still on c_ovq when the device is unbound,
free_buf() reads past the ports_device object.

KASAN reported the use through:

	free_buf
	remove_vqs
	virtcons_remove
	unbind_store

Drain c_ovq without freeing. The packet lives in portdev and is released
with it.

Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
Cc: stable@vger.kernel.org
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
 drivers/char/virtio_console.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9a33217c68d9..6e9bf064826e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1886,10 +1886,24 @@ static void remove_vqs(struct ports_device *portdev)
 
 	virtio_device_for_each_vq(portdev->vdev, vq) {
 		struct port_buffer *buf;
+		unsigned int len;
 
-		flush_bufs(vq, true);
-		while ((buf = virtqueue_detach_unused_buf(vq)))
-			free_buf(buf, true);
+		/*
+		 * c_ovq cookies are &portdev->cpkt, not port_buffer.
+		 * Detach them but do not free_buf().
+		 */
+		if (vq == portdev->c_ovq) {
+			spin_lock(&portdev->c_ovq_lock);
+			while (virtqueue_get_buf(vq, &len))
+				;
+			while (virtqueue_detach_unused_buf(vq))
+				;
+			spin_unlock(&portdev->c_ovq_lock);
+		} else {
+			flush_bufs(vq, true);
+			while ((buf = virtqueue_detach_unused_buf(vq)))
+				free_buf(buf, true);
+		}
 		cond_resched();
 	}
 	portdev->vdev->config->del_vqs(portdev->vdev);
-- 
2.34.1

Re: [PATCH] virtio_console: do not free control-out buffers on remove

[Jia Jia]

>
> __send_control_msg() publishes &portdev->cpkt as the control-out
> virtqueue cookie. remove_vqs() walks every virtqueue and passes leftover
> cookies to free_buf(), which treats them as struct port_buffer and
> reads sgpages.
>
> If a control message is still on c_ovq when the device is unbound,
> free_buf() reads past the ports_device object.
>
> KASAN reported the use through:
>
>         free_buf
>         remove_vqs
>         virtcons_remove
>         unbind_store
>
> Drain c_ovq without freeing. The packet lives in portdev and is released
> with it.
>
> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> ---
>  drivers/char/virtio_console.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 9a33217c68d9..6e9bf064826e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1886,10 +1886,24 @@ static void remove_vqs(struct ports_device *portdev)
>
>         virtio_device_for_each_vq(portdev->vdev, vq) {
>                 struct port_buffer *buf;
> +               unsigned int len;
>
> -               flush_bufs(vq, true);
> -               while ((buf = virtqueue_detach_unused_buf(vq)))
> -                       free_buf(buf, true);
> +               /*
> +                * c_ovq cookies are &portdev->cpkt, not port_buffer.
> +                * Detach them but do not free_buf().
> +                */
> +               if (vq == portdev->c_ovq) {

The non-multiport case was not considered here; fixed in PATCH v2.

> +                       spin_lock(&portdev->c_ovq_lock);
> +                       while (virtqueue_get_buf(vq, &len))
> +                               ;
> +                       while (virtqueue_detach_unused_buf(vq))
> +                               ;
> +                       spin_unlock(&portdev->c_ovq_lock);
> +               } else {
> +                       flush_bufs(vq, true);
> +                       while ((buf = virtqueue_detach_unused_buf(vq)))
> +                               free_buf(buf, true);
> +               }
>                 cond_resched();
>         }
>         portdev->vdev->config->del_vqs(portdev->vdev);
> --
> 2.34.1