Re: [PATCH v1 2/6] virtio console: Harden port adding
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2023-01-27 11:03:39
Also in:
lkml
On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
Greg Kroah-Hartman [off-list ref] writes:quoted
Then you need to copy it out once, and then only deal with the local copy. Otherwise you have an incomplete snapshot.Ok, would you be partial to something like this:quoted
From 1bc9bb84004154376c2a0cf643d53257da6d1cd7 Mon Sep 17 00:00:00 2001From: Alexander Shishkin <alexander.shishkin@linux.intel.com> Date: Thu, 19 Jan 2023 21:59:02 +0200 Subject: [PATCH] virtio console: Keep a local copy of the control structure When handling control messages, instead of peeking at the device memory to obtain bits of the control structure,
Except the message makes it seem that we are getting data from device memory, when we do nothing of the kind.
take a snapshot of it once and use it instead, to prevent it from changing under us. This avoids races between port id validation and control event decoding, which can lead to, for example, a NULL dereference in port removal of a nonexistent port. The control structure is small enough (8 bytes) that it can be cached directly on the stack.
I still have no real idea why we want a copy here. If device can poke anywhere at memory then it can crash kernel anyway. If there's a bounce buffer or an iommu or some other protection in place, then this memory can no longer change by the time we look at it.
quoted hunk
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Amit Shah <amit@kernel.org> --- drivers/char/virtio_console.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 6a821118d553..42be0991a72f 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c@@ -1559,23 +1559,24 @@ static void handle_control_message(struct virtio_device *vdev, struct ports_device *portdev, struct port_buffer *buf) { - struct virtio_console_control *cpkt; + struct virtio_console_control cpkt; struct port *port; size_t name_size; int err; - cpkt = (struct virtio_console_control *)(buf->buf + buf->offset); + /* Keep a local copy of the control structure */ + memcpy(&cpkt, buf->buf + buf->offset, sizeof(cpkt)); - port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id)); + port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt.id)); if (!port && - cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) { + cpkt.event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) { /* No valid header at start of buffer. Drop it. */ dev_dbg(&portdev->vdev->dev, - "Invalid index %u in control packet\n", cpkt->id); + "Invalid index %u in control packet\n", cpkt.id); return; } - switch (virtio16_to_cpu(vdev, cpkt->event)) { + switch (virtio16_to_cpu(vdev, cpkt.event)) { case VIRTIO_CONSOLE_PORT_ADD: if (port) { dev_dbg(&portdev->vdev->dev,@@ -1583,21 +1584,21 @@ static void handle_control_message(struct virtio_device *vdev, send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1); break; } - if (virtio32_to_cpu(vdev, cpkt->id) >= + if (virtio32_to_cpu(vdev, cpkt.id) >= portdev->max_nr_ports) { dev_warn(&portdev->vdev->dev, "Request for adding port with " "out-of-bound id %u, max. supported id: %u\n", - cpkt->id, portdev->max_nr_ports - 1); + cpkt.id, portdev->max_nr_ports - 1); break; } - add_port(portdev, virtio32_to_cpu(vdev, cpkt->id)); + add_port(portdev, virtio32_to_cpu(vdev, cpkt.id)); break; case VIRTIO_CONSOLE_PORT_REMOVE: unplug_port(port); break; case VIRTIO_CONSOLE_CONSOLE_PORT: - if (!cpkt->value) + if (!cpkt.value) break; if (is_console_port(port)) break;@@ -1618,7 +1619,7 @@ static void handle_control_message(struct virtio_device *vdev, if (!is_console_port(port)) break; - memcpy(&size, buf->buf + buf->offset + sizeof(*cpkt), + memcpy(&size, buf->buf + buf->offset + sizeof(cpkt), sizeof(size)); set_console_size(port, size.rows, size.cols);@@ -1627,7 +1628,7 @@ static void handle_control_message(struct virtio_device *vdev, break; } case VIRTIO_CONSOLE_PORT_OPEN: - port->host_connected = virtio16_to_cpu(vdev, cpkt->value); + port->host_connected = virtio16_to_cpu(vdev, cpkt.value); wake_up_interruptible(&port->waitqueue); /* * If the host port got closed and the host had any@@ -1658,7 +1659,7 @@ static void handle_control_message(struct virtio_device *vdev, * Skip the size of the header and the cpkt to get the size * of the name that was sent */ - name_size = buf->len - buf->offset - sizeof(*cpkt) + 1; + name_size = buf->len - buf->offset - sizeof(cpkt) + 1; port->name = kmalloc(name_size, GFP_KERNEL); if (!port->name) {@@ -1666,7 +1667,7 @@ static void handle_control_message(struct virtio_device *vdev, "Not enough space to store port name\n"); break; } - strncpy(port->name, buf->buf + buf->offset + sizeof(*cpkt), + strncpy(port->name, buf->buf + buf->offset + sizeof(cpkt), name_size - 1); port->name[name_size - 1] = 0;-- 2.39.0
_______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization