In handle_control_message(), we look at the ->event field twice, which
gives a malicious VMM a window in which to switch it from PORT_ADD to
PORT_REMOVE, triggering a null dereference further down the line:

RIP: 0010:spin_lock_irq ./include/linux/spinlock.h:388
RIP: 0010:unplug_port+0x9/0x150 drivers/char/virtio_console.c:1512
Call Trace:
 handle_control_message+0x108/0x2c0 drivers/char/virtio_console.c:1600
 elfcorehdr_read+0x40/0x40 ??:?
 process_one_work+0x1b4/0x310 kernel/workqueue.c:2297
 worker_thread+0x5c/0x3a0 kernel/workqueue.c:2444
 kthread+0x120/0x140 kernel/kthread.c:319
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

Read the event code once instead, basing all following decisions on the
same value.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Amit Shah <amit@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/char/virtio_console.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6599c2956ba4..62f69f949cb7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1563,22 +1563,22 @@ static void handle_control_message(struct virtio_device *vdev,
 	struct port *port;
 	size_t name_size;
 	int err;
-	unsigned id;
+	unsigned id, event;
 
 	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
 
-	/* Make sure the host cannot change id under us */
+	/* Make sure the host cannot change id or event under us */
 	id = virtio32_to_cpu(vdev, READ_ONCE(cpkt->id));
+	event = virtio16_to_cpu(vdev, cpkt->event);
 	port = find_port_by_id(portdev, id);
-	if (!port &&
-	    cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) {
+	if (!port && event != 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);
 		return;
 	}
 
-	switch (virtio16_to_cpu(vdev, cpkt->event)) {
+	switch (event) {
 	case VIRTIO_CONSOLE_PORT_ADD:
 		if (port) {
 			dev_dbg(&portdev->vdev->dev,
-- 
2.39.0

On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> In handle_control_message(), we look at the ->event field twice, which
> gives a malicious VMM a window in which to switch it from PORT_ADD to
> PORT_REMOVE, triggering a null dereference further down the line:

How is the other VMM have full control over the full message here?
Shouldn't this all have been copied into our local memory if we are
going to be poking around in it?  Like I mentioned in my other review,
copy it all once and then parse it.  Don't try to mess with individual
fields one at a time otherwise that way lies madness...

thanks,

greg k-h

On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> > In handle_control_message(), we look at the ->event field twice, which
> > gives a malicious VMM a window in which to switch it from PORT_ADD to
> > PORT_REMOVE, triggering a null dereference further down the line:
> 
> How is the other VMM have full control over the full message here?
> Shouldn't this all have been copied into our local memory if we are
> going to be poking around in it?  Like I mentioned in my other review,
> copy it all once and then parse it.  Don't try to mess with individual
> fields one at a time otherwise that way lies madness...
> 
> thanks,
> 
> greg k-h

I agree and in fact, it is *already* copied since with malicious
device we generally use a bounce buffer.
Having said that, the patch is actually a cleanup, e.g. it's clearer
to byte-swap only once.
Just don't oversell it as a security thing.


-- 
MST

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman wrote:
>> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
>> > In handle_control_message(), we look at the ->event field twice, which
>> > gives a malicious VMM a window in which to switch it from PORT_ADD to
>> > PORT_REMOVE, triggering a null dereference further down the line:
>> 
>> How is the other VMM have full control over the full message here?
>> Shouldn't this all have been copied into our local memory if we are
>> going to be poking around in it?  Like I mentioned in my other review,
>> copy it all once and then parse it.  Don't try to mess with individual
>> fields one at a time otherwise that way lies madness...
>> 
>> thanks,
>> 
>> greg k-h
>
> I agree and in fact, it is *already* copied since with malicious
> device we generally use a bounce buffer.

Right, but the code should probably be able to handle bad input on its
own, or what do you think?

> Having said that, the patch is actually a cleanup, e.g. it's clearer
> to byte-swap only once.
> Just don't oversell it as a security thing.

Well, security was the original motivation, so that's what it said in
the commit message. But we settled on [0] yesterday with Greg, which
would replace this patch and 2/6.

[0] https://lore.kernel.org/all/87a62eqo4h.fsf@ubik.fi.intel.com/

Regards,
--
Alex

On Fri, Jan 20, 2023 at 06:41:27PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman wrote:
> >> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> >> > In handle_control_message(), we look at the ->event field twice, which
> >> > gives a malicious VMM a window in which to switch it from PORT_ADD to
> >> > PORT_REMOVE, triggering a null dereference further down the line:
> >> 
> >> How is the other VMM have full control over the full message here?
> >> Shouldn't this all have been copied into our local memory if we are
> >> going to be poking around in it?  Like I mentioned in my other review,
> >> copy it all once and then parse it.  Don't try to mess with individual
> >> fields one at a time otherwise that way lies madness...
> >> 
> >> thanks,
> >> 
> >> greg k-h
> >
> > I agree and in fact, it is *already* copied since with malicious
> > device we generally use a bounce buffer.
> 
> Right, but the code should probably be able to handle bad input on its
> own, or what do you think?

Basically I think it's ok to look at the same field twice unless
it's mapped as dma coherent. Is that what you are asking about?

> > Having said that, the patch is actually a cleanup, e.g. it's clearer
> > to byte-swap only once.
> > Just don't oversell it as a security thing.
> 
> Well, security was the original motivation, so that's what it said in
> the commit message. But we settled on [0] yesterday with Greg, which
> would replace this patch and 2/6.
> 
> [0] https://lore.kernel.org/all/87a62eqo4h.fsf@ubik.fi.intel.com/
> 
> Regards,

At this point I will drop this series and pls post new series
with just the stuff you want included. Include acks if patches
are unchanged.

Thanks!

> --
> Alex

"Michael S. Tsirkin" <mst@redhat.com> writes:

> At this point I will drop this series and pls post new series
> with just the stuff you want included. Include acks if patches
> are unchanged.

Will do, thanks for the review!

Regards,
--
Alex