[v1,0/6] Harden a few virtio bits

  Hi,

Here are 6 patches that harden console, net and 9p drivers against
various malicious host input as well as close a bounds check bypass
in the split virtio ring.

Changes since previous version:
 * Added Christian's R-B to 3/6
 * Added a speculation fix per Michael's comment on the cover letter
 * CC'ing lkml

Alexander Shishkin (3):
  virtio console: Harden control message handling
  virtio_net: Guard against buffer length overflow in
    xdp_linearize_page()
  virtio_ring: Prevent bounds check bypass on descriptor index

Andi Kleen (3):
  virtio console: Harden multiport against invalid host input
  virtio console: Harden port adding
  virtio 9p: Fix an overflow

 drivers/char/virtio_console.c | 19 ++++++++++++-------
 drivers/net/virtio_net.c      |  4 +++-
 drivers/virtio/virtio_ring.c  |  3 +++
 net/9p/trans_virtio.c         |  2 +-
 4 files changed, 19 insertions(+), 9 deletions(-)
  

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>> 
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -1843,6 +1843,9 @@ static int init_vqs(struct ports_device *portdev)
>>  	int err;
>>  
>>  	nr_ports = portdev->max_nr_ports;
>> +	if (use_multiport(portdev) && nr_ports < 1)
>> +		return -EINVAL;
>> +
>>  	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
>>  
>>  	vqs = kmalloc_array(nr_queues, sizeof(struct virtqueue *), GFP_KERNEL);
>> -- 
>> 2.39.0
>> 
>
> Why did I only get a small subset of these patches?

I did what get_maintainer told me. Would you like to be CC'd on the
whole thing?

> And why is the whole thread not on lore.kernel.org?

That is a mystery, some wires got crossed between my smtp and vger. I
bounced the series to lkml just now and at least some of it seems to
have landed on lore.

> And the term "hardening" is marketing fluff.   Just say, "properly parse
> input" or something like that, as what you are doing is fixing
> assumptions about the data here, not causing anything to be more (or
> less) secure.
>
> But, this still feels wrong.  Why is this happening here, in init_vqs()
> and not in the calling function that already did a bunch of validation
> of the ports and the like?  Are those checks not enough?  if not, fix it
> there, don't spread it out all over the place...

Good point! And there happens to already be 28962ec595d70 that takes
care of exactly this case. I totally missed it.

Regards,
--
Alex
  

On Thu, Jan 19, 2023 at 07:48:35PM +0200, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Thu, Jan 19, 2023 at 03:57:17PM +0200, Alexander Shishkin wrote:
> >> From: Andi Kleen <ak@linux.intel.com>
> >> 
> >> 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 <ak@linux.intel.com>
> >> 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, 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.
> 
> This is the device memory or the VM host memory, not userspace or
> another process. And it can change under us willy-nilly.

Then you need to copy it out once, and then only deal with the local
copy.  Otherwise you have an incomplete snapshot.

> The thing is, we only need to cache two things to correctly process the
> request. Copying everything, on the other hand, would involve the entire
> buffer, not just the *cpkt, but also stuff that follows, which also
> differs between different event types. And we also don't care if the
> rest of it changes under us.

That feels broken if you do not "trust" that other side.  And what
prevents the buffer from changing after you validated the other part?

For virtio, I thought you always implied that you did trust the other
side, when has that changed?  Where was that new security model for the
kernel discussed?

Are you sure this is even viable?  What is the threat model you are
attempting to add to the driver here?

> > Otherwise this is going to be impossible to actually maintain over
> > time...
> 
> An 'id' can't possibly be worse to maintain than multiple instances of
> 'virtio32_to_cpu(vdev, cpkt->id)' sprinkled around the code.

Again, copy what you want out and then act on that.  If it can change
under you, and you do not trust it, then you have to work only on a
snapshot that you have verified.

thanks,

greg k-h
  

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Thu, Jan 19, 2023 at 08:52:02PM +0200, Alexander Shishkin wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > Why did I only get a small subset of these patches?
>> 
>> I did what get_maintainer told me. Would you like to be CC'd on the
>> whole thing?
>
> If you only cc: me on a portion of the series, I guess you only want me
> to apply a portion of it?  if so, why is it a longer series?

I was expecting that this series will eventually go in via the virtio
maintainers, assuming you can give your acks to the char bits.

Or, I can split off the char bits and send them to you
separately. Whichever makes the most sense.

>> > But, this still feels wrong.  Why is this happening here, in init_vqs()
>> > and not in the calling function that already did a bunch of validation
>> > of the ports and the like?  Are those checks not enough?  if not, fix it
>> > there, don't spread it out all over the place...
>> 
>> Good point! And there happens to already be 28962ec595d70 that takes
>> care of exactly this case. I totally missed it.
>
> So this series is not needed?  Or just this one?

Just this one.

Regards,
--
Alex
  

Greg Kroah-Hartman <gregkh@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@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, 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.

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;
  

On Thu, Jan 19, 2023 at 03:57:15PM +0200, Alexander Shishkin wrote:
> Hi,
> 
> Here are 6 patches that harden console, net and 9p drivers against
> various malicious host input as well as close a bounds check bypass
> in the split virtio ring.

Hardening against buggy devices is one thing,
Hardening against malicious devices is another.
Which is this?
If really malicious, aren't there any spectre considerations here?
I am for example surprised not to find anything addressing
spectre v1 nor any uses of array_index_nospec here.


> Changes since previous version:
>  * Added Christian's R-B to 3/6
>  * Added a speculation fix per Michael's comment on the cover letter
>  * CC'ing lkml
> 
> Alexander Shishkin (3):
>   virtio console: Harden control message handling
>   virtio_net: Guard against buffer length overflow in
>     xdp_linearize_page()
>   virtio_ring: Prevent bounds check bypass on descriptor index
> 
> Andi Kleen (3):
>   virtio console: Harden multiport against invalid host input
>   virtio console: Harden port adding
>   virtio 9p: Fix an overflow
> 
>  drivers/char/virtio_console.c | 19 ++++++++++++-------
>  drivers/net/virtio_net.c      |  4 +++-
>  drivers/virtio/virtio_ring.c  |  3 +++
>  net/9p/trans_virtio.c         |  2 +-
>  4 files changed, 19 insertions(+), 9 deletions(-)
> 
> -- 
> 2.39.0
  

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

> On Thu, Jan 19, 2023 at 03:57:15PM +0200, Alexander Shishkin wrote:
>> Hi,
>> 
>> Here are 6 patches that harden console, net and 9p drivers against
>> various malicious host input as well as close a bounds check bypass
>> in the split virtio ring.
>
> Hardening against buggy devices is one thing,
> Hardening against malicious devices is another.
> Which is this?

Well, the big difference is the intent, but buggy input is buggy input,
they've got that in common and we're trying to deal with it here.

The motivation for this patchset is protecting against malicious
devices.

> If really malicious, aren't there any spectre considerations here?
> I am for example surprised not to find anything addressing
> spectre v1 nor any uses of array_index_nospec here.

That's strange, patch 6/6 is exactly that. There's probably more coming
in the future as the analysis and audit progress.

Regards,
--
Alex
  

On Fri, Jan 20, 2023 at 02:32:09PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jan 19, 2023 at 03:57:15PM +0200, Alexander Shishkin wrote:
> >> Hi,
> >> 
> >> Here are 6 patches that harden console, net and 9p drivers against
> >> various malicious host input as well as close a bounds check bypass
> >> in the split virtio ring.
> >
> > Hardening against buggy devices is one thing,
> > Hardening against malicious devices is another.
> > Which is this?
> 
> Well, the big difference is the intent, but buggy input is buggy input,
> they've got that in common and we're trying to deal with it here.
> 
> The motivation for this patchset is protecting against malicious
> devices.
> 
> > If really malicious, aren't there any spectre considerations here?
> > I am for example surprised not to find anything addressing
> > spectre v1 nor any uses of array_index_nospec here.
> 
> That's strange, patch 6/6 is exactly that. There's probably more coming
> in the future as the analysis and audit progress.
> 
> Regards,

Oh I see, didn't get it for some reason. Pulled it from lore now.

> --
> Alex