Michael S. Tsirkin
2023-Jan-27 11:02 UTC
[PATCH v1 2/6] virtio console: Harden port adding
On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:> Greg Kroah-Hartman <gregkh at linuxfoundation.org> writes: > > > 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: > > >From 1bc9bb84004154376c2a0cf643d53257da6d1cd7 Mon Sep 17 00:00:00 2001 > From: Alexander Shishkin <alexander.shishkin at 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.> Signed-off-by: Alexander Shishkin <alexander.shishkin at linux.intel.com> > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org> > Cc: Arnd Bergmann <arnd at arndb.de> > Cc: Amit Shah <amit at 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