[PATCH] virtio: console: check the control packet size before parsing it

[PATCH] virtio: console: check the control packet size before parsing it

[Guanglei Zhu]

The control queue handler hands whatever the host wrote to the
control queue straight to the parser:

	buf->len = min_t(size_t, len, buf->size);
	buf->offset = 0;

	handle_control_message(vq->vdev, portdev, buf);

handle_control_message() casts the start of the buffer to struct
virtio_console_control and reads the id, event and value fields
without ever looking at buf->len. A control packet shorter than the
structure makes the driver parse whatever bytes were left in the
reused buffer as a control header, and the uninitialized event then
drives the port state machine.

Drop control packets that are too short to hold the header.

Fixes: 17634ba25544 ("virtio: console: Add a new MULTIPORT feature, support for generic ports")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/char/virtio_console.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 019bcae81af5..5e2a4403dbf8 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1597,6 +1597,12 @@ static void handle_control_message(struct virtio_device *vdev,
 	size_t name_size;
 	int err;
 
+	if (buf->len < sizeof(struct virtio_console_control)) {
+		dev_dbg(&portdev->vdev->dev,
+			"Invalid control packet size %zu\n", buf->len);
+		return;
+	}
+
 	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
 
 	port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id));
-- 
2.50.1

Re: [PATCH] virtio: console: check the control packet size before parsing it

[sashiko-bot]

> The control queue handler hands whatever the host wrote to the
> control queue straight to the parser:
> 
> 	buf->len = min_t(size_t, len, buf->size);
> 	buf->offset = 0;
> 
> 	handle_control_message(vq->vdev, portdev, buf);
> 
> handle_control_message() casts the start of the buffer to struct
> virtio_console_control and reads the id, event and value fields
> without ever looking at buf->len. A control packet shorter than the
> structure makes the driver parse whatever bytes were left in the
> reused buffer as a control header, and the uninitialized event then
> drives the port state machine.
> 
> Drop control packets that are too short to hold the header.
> 
> Fixes: 17634ba25544 ("virtio: console: Add a new MULTIPORT feature, support for generic ports")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103084156.426810-1-zhugl3@xiaopeng.com?part=1

Re: [PATCH] virtio: console: check the control packet size before parsing it

[Michael S. Tsirkin]

On Tue, Nov 03, 2026 at 04:41:56PM +0800, Guanglei Zhu wrote:
> The control queue handler hands whatever the host wrote to the
> control queue straight to the parser:
> 
> 	buf->len = min_t(size_t, len, buf->size);
> 	buf->offset = 0;
> 
> 	handle_control_message(vq->vdev, portdev, buf);
> 
> handle_control_message() casts the start of the buffer to struct
> virtio_console_control and reads the id, event and value fields
> without ever looking at buf->len. A control packet shorter than the
> structure

That's a broken device.

> makes the driver parse whatever bytes were left in the
> reused buffer as a control header, and the uninitialized event then
> drives the port state machine.

So what?

> Drop control packets that are too short to hold the header.

And then what?

> Fixes: 17634ba25544 ("virtio: console: Add a new MULTIPORT feature, support for generic ports")
> Cc: stable@vger.kernel.org

Not clear why this is needed on stable.


> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
> ---
>  drivers/char/virtio_console.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 019bcae81af5..5e2a4403dbf8 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1597,6 +1597,12 @@ static void handle_control_message(struct virtio_device *vdev,
>  	size_t name_size;
>  	int err;
>  
> +	if (buf->len < sizeof(struct virtio_console_control)) {
> +		dev_dbg(&portdev->vdev->dev,
> +			"Invalid control packet size %zu\n", buf->len);
> +		return;
> +	}
> +
>  	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
>  
>  	port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id));
> -- 
> 2.50.1
> 

Re: [PATCH] virtio: console: check the control packet size before parsing it

[Guanglei Zhu]

You are right, the impact statement does not hold up: the reused
buffer still contains a previously valid control message, so a short
packet makes the driver parse stale - not attacker-controlled - data,
and that is not worth carrying an extra check for.  Please drop this
patch.

Sorry for the noise, and thanks for the review.

Re: [PATCH] virtio: console: check the control packet size before parsing it

[Michael S. Tsirkin]

On Tue, Nov 03, 2026 at 07:17:34PM +0800, Guanglei Zhu wrote:
> You are right, the impact statement does not hold up: the reused
> buffer still contains a previously valid control message,

Not always, the buffers are allocated with GFP_KERNEL on the
1st pass.

> so a short
> packet makes the driver parse stale - not attacker-controlled - data,

Who is the attacker here the device? it is absolutely
controlling all of virtio_console_control.

> and that is not worth carrying an extra check for.  Please drop this
> patch.
> 
> Sorry for the noise, and thanks for the review.

I ask that you please think critically not just go back and forth on it
like an LLM.

-- 
MST

Re: [PATCH] virtio: console: check the control packet size before parsing it

[Guanglei Zhu]

Fair enough.  I was wrong about the buffer contents and about who
controls the stream.  The real issue is that a bad length just makes
the driver act on garbage values, and nothing in the switch derives
a memory access from them, so the patch is hardening, not a fix.
Keeping it withdrawn.