On Thu, Jan 19, 2023 at 03:57:17PM +0200, Alexander Shishkin wrote: > From: Andi Kleen <[email protected]> > > The ADD_PORT operation reads and sanity checks the port id multiple > times from the untrusted host. This is not safe because a malicious > host could change it between reads. > > Read the port id only once and cache it for subsequent uses. > > Signed-off-by: Andi Kleen <[email protected]> > Signed-off-by: Alexander Shishkin <[email protected]> > Cc: Amit Shah <[email protected]> > Cc: Arnd Bergmann <[email protected]> > Cc: Greg Kroah-Hartman <[email protected]> > --- > drivers/char/virtio_console.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index f4fd5fe7cd3a..6599c2956ba4 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1563,10 +1563,13 @@ static void handle_control_message(struct > virtio_device *vdev, > struct port *port; > size_t name_size; > int err; > + unsigned id; > > cpkt = (struct virtio_console_control *)(buf->buf + buf->offset); > > - port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id)); > + /* Make sure the host cannot change id under us */ > + id = virtio32_to_cpu(vdev, READ_ONCE(cpkt->id));
Why READ_ONCE()? And how can it change under us? Is the message still under control of the "host"? If so, that feels wrong as this is all in kernel memory, not userspace memory right? If you are dealing with memory from a different process that you do not trust, then you need to copy EVERYTHING at once. Don't piece-meal copy bits and bobs in all different places please. Do it once and then parse the local structure properly. Otherwise this is going to be impossible to actually maintain over time... thanks, greg k-h _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
