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

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

[Alexander Shishkin]

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(-)

-- 
2.39.0

[PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Alexander Shishkin]

From: Andi Kleen <ak@linux.intel.com>

It's possible for the host to set the multiport flag, but pass in
0 multiports, which results in:

BUG: KASAN: slab-out-of-bounds in init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
Write of size 8 at addr ffff888001cc24a0 by task swapper/1

CPU: 0 PID: 1 Comm: swapper Not tainted 5.15.0-rc1-140273-gaab0bb9fbaa1-dirty #588
Call Trace:
 init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
 virtcons_probe+0x1a3/0x5b0 drivers/char/virtio_console.c:2042
 virtio_dev_probe+0x2b9/0x500 drivers/virtio/virtio.c:263
 call_driver_probe drivers/base/dd.c:515
 really_probe+0x1c9/0x5b0 drivers/base/dd.c:601
 really_probe_debug drivers/base/dd.c:694
 __driver_probe_device+0x10d/0x1f0 drivers/base/dd.c:754
 driver_probe_device+0x68/0x150 drivers/base/dd.c:786
 __driver_attach+0xca/0x200 drivers/base/dd.c:1145
 bus_for_each_dev+0x108/0x190 drivers/base/bus.c:301
 driver_attach+0x30/0x40 drivers/base/dd.c:1162
 bus_add_driver+0x325/0x3c0 drivers/base/bus.c:618
 driver_register+0xf3/0x1d0 drivers/base/driver.c:171
...

Add a suitable sanity check.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6a821118d553..f4fd5fe7cd3a 100644
--- 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

Re: [PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Greg Kroah-Hartman]

On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> It's possible for the host to set the multiport flag, but pass in
> 0 multiports, which results in:
> 
> BUG: KASAN: slab-out-of-bounds in init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
> Write of size 8 at addr ffff888001cc24a0 by task swapper/1
> 
> CPU: 0 PID: 1 Comm: swapper Not tainted 5.15.0-rc1-140273-gaab0bb9fbaa1-dirty #588
> Call Trace:
>  init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
>  virtcons_probe+0x1a3/0x5b0 drivers/char/virtio_console.c:2042
>  virtio_dev_probe+0x2b9/0x500 drivers/virtio/virtio.c:263
>  call_driver_probe drivers/base/dd.c:515
>  really_probe+0x1c9/0x5b0 drivers/base/dd.c:601
>  really_probe_debug drivers/base/dd.c:694
>  __driver_probe_device+0x10d/0x1f0 drivers/base/dd.c:754
>  driver_probe_device+0x68/0x150 drivers/base/dd.c:786
>  __driver_attach+0xca/0x200 drivers/base/dd.c:1145
>  bus_for_each_dev+0x108/0x190 drivers/base/bus.c:301
>  driver_attach+0x30/0x40 drivers/base/dd.c:1162
>  bus_add_driver+0x325/0x3c0 drivers/base/bus.c:618
>  driver_register+0xf3/0x1d0 drivers/base/driver.c:171
> ...
> 
> Add a suitable sanity check.
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 6a821118d553..f4fd5fe7cd3a 100644
> --- 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?

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

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...

thanks,

greg k-h

Re: [PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Alexander Shishkin]

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

Re: [PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Greg Kroah-Hartman]

On Thu, Jan 19, 2023 at 08:52:02PM +0200, Alexander Shishkin wrote:
> 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?

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?

> > 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.

So this series is not needed?  Or just this one?

greg k-h

Re: [PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Alexander Shishkin]

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

Re: [PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Michael S. Tsirkin]

On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> It's possible for the host to set the multiport flag, but pass in
> 0 multiports, which results in:
> 
> BUG: KASAN: slab-out-of-bounds in init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
> Write of size 8 at addr ffff888001cc24a0 by task swapper/1
> 
> CPU: 0 PID: 1 Comm: swapper Not tainted 5.15.0-rc1-140273-gaab0bb9fbaa1-dirty #588
> Call Trace:
>  init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
>  virtcons_probe+0x1a3/0x5b0 drivers/char/virtio_console.c:2042
>  virtio_dev_probe+0x2b9/0x500 drivers/virtio/virtio.c:263
>  call_driver_probe drivers/base/dd.c:515
>  really_probe+0x1c9/0x5b0 drivers/base/dd.c:601
>  really_probe_debug drivers/base/dd.c:694
>  __driver_probe_device+0x10d/0x1f0 drivers/base/dd.c:754
>  driver_probe_device+0x68/0x150 drivers/base/dd.c:786
>  __driver_attach+0xca/0x200 drivers/base/dd.c:1145
>  bus_for_each_dev+0x108/0x190 drivers/base/bus.c:301
>  driver_attach+0x30/0x40 drivers/base/dd.c:1162
>  bus_add_driver+0x325/0x3c0 drivers/base/bus.c:618
>  driver_register+0xf3/0x1d0 drivers/base/driver.c:171
> ...
> 
> Add a suitable sanity check.
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 6a821118d553..f4fd5fe7cd3a 100644
> --- 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);

Weird.  Don't we already check for that?

        /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
        if (!is_rproc_serial(vdev) &&
            virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
                                 struct virtio_console_config, max_nr_ports,
                                 &portdev->max_nr_ports) == 0) {
                if (portdev->max_nr_ports == 0 ||
                    portdev->max_nr_ports > VIRTCONS_MAX_PORTS) {
                        dev_err(&vdev->dev,
                                "Invalidate max_nr_ports %d",
                                portdev->max_nr_ports);
                        err = -EINVAL;
                        goto free;
                }
                multiport = true;
        }




> -- 
> 2.39.0

Re: [PATCH v1 1/6] virtio console: Harden multiport against invalid host input

[Alexander Shishkin]

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

> Weird.  Don't we already check for that?
>
>         /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
>         if (!is_rproc_serial(vdev) &&
>             virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
>                                  struct virtio_console_config, max_nr_ports,
>                                  &portdev->max_nr_ports) == 0) {
>                 if (portdev->max_nr_ports == 0 ||
>                     portdev->max_nr_ports > VIRTCONS_MAX_PORTS) {
>                         dev_err(&vdev->dev,
>                                 "Invalidate max_nr_ports %d",
>                                 portdev->max_nr_ports);
>                         err = -EINVAL;
>                         goto free;
>                 }
>                 multiport = true;
>         }

Yes, I missed this earlier. I'll drop this patch.

Thanks,
--
Alex

[PATCH v1 2/6] virtio console: Harden port adding

[Alexander Shishkin]

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));
+	port = find_port_by_id(portdev, id);
 	if (!port &&
 	    cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) {
 		/* No valid header at start of buffer.  Drop it. */
@@ -1583,15 +1586,14 @@ 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) >=
-		    portdev->max_nr_ports) {
+		if (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);
 			break;
 		}
-		add_port(portdev, virtio32_to_cpu(vdev, cpkt->id));
+		add_port(portdev, id);
 		break;
 	case VIRTIO_CONSOLE_PORT_REMOVE:
 		unplug_port(port);
-- 
2.39.0

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Greg Kroah-Hartman]

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.

Otherwise this is going to be impossible to actually maintain over
time...

thanks,

greg k-h

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Alexander Shishkin]

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.

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.

> 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.

Thanks,
--
Alex

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Greg Kroah-Hartman]

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

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Alexander Shishkin]

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;
 
-- 
2.39.0

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Greg Kroah-Hartman]

On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
> 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(-)

Yes, this looks much better, thanks!

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

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Michael S. Tsirkin]

On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
> 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,

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@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

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Alexander Shishkin]

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

> On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
>> 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.

We can be, see below.

>> 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.

We can have shared pages between the host and guest without bounce
buffers in between, so they can be both looking directly at the same
page.

Regards,
--
Alex

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Michael S. Tsirkin]

On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
> >> 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.
> 
> We can be, see below.
> 
> >> 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.
> 
> We can have shared pages between the host and guest without bounce
> buffers in between, so they can be both looking directly at the same
> page.
> 
> Regards,

How does this configuration work? What else is in this page?

> --
> Alex

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Alexander Shishkin]

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

> On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
>> >> 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.
>> 
>> We can be, see below.
>> 
>> >> 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.
>> 
>> We can have shared pages between the host and guest without bounce
>> buffers in between, so they can be both looking directly at the same
>> page.
>> 
>> Regards,
>
> How does this configuration work? What else is in this page?

So, for example in TDX, you have certain pages as "shared", as in
between guest and hypervisor. You can have virtio ring(s) in such
pages. It's likely that there'd be a swiotlb buffer there instead, but
sharing pages between host virtio and guest virtio drivers is possible.

Apologies if the language is confusing, I hope I'm answering the
question.

Regards,
--
Alex

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Greg Kroah-Hartman]

On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
> >> >> 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.
> >> 
> >> We can be, see below.
> >> 
> >> >> 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.
> >> 
> >> We can have shared pages between the host and guest without bounce
> >> buffers in between, so they can be both looking directly at the same
> >> page.
> >> 
> >> Regards,
> >
> > How does this configuration work? What else is in this page?
> 
> So, for example in TDX, you have certain pages as "shared", as in
> between guest and hypervisor. You can have virtio ring(s) in such
> pages. It's likely that there'd be a swiotlb buffer there instead, but
> sharing pages between host virtio and guest virtio drivers is possible.

If it is shared, then what does this mean?  Do we then need to copy
everything out of that buffer first before doing anything with it
because the data could change later on?  Or do we not trust anything in
it at all and we throw it away?  Or something else (trust for a short
while and then we don't?)

Please be specific as to what you want to see happen here, and why.

thanks,

greg k-h

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Alexander Shishkin]

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

> On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
>> >> We can have shared pages between the host and guest without bounce
>> >> buffers in between, so they can be both looking directly at the same
>> >> page.
>> >> 
>> >> Regards,
>> >
>> > How does this configuration work? What else is in this page?
>> 
>> So, for example in TDX, you have certain pages as "shared", as in
>> between guest and hypervisor. You can have virtio ring(s) in such
>> pages. It's likely that there'd be a swiotlb buffer there instead, but
>> sharing pages between host virtio and guest virtio drivers is possible.
>
> If it is shared, then what does this mean?  Do we then need to copy
> everything out of that buffer first before doing anything with it
> because the data could change later on?  Or do we not trust anything in
> it at all and we throw it away?  Or something else (trust for a short
> while and then we don't?)

The first one, we need a consistent view of the metadata (the ckpt in
this case), so we take a snapshot of it. Then, we validate it (because
we don't trust it) to be correct. If it is not, we discard it, otherwise
we act on it. Since this is a ring, we just move on to the next record
if there is one.

Meanwhile, in the shared page, it can change from correct to incorrect,
but it won't affect us because we have this consistent view at the
moment the snapshot was taken.

> Please be specific as to what you want to see happen here, and why.

For example, if we get a control message to add a port and
cpkt->event==PORT_ADD, we skip validation of cpkt->id (port id), because
we're intending to add a new one. At this point, the device can change
cpkt->event to PORT_REMOVE, which does require a valid cpkt->id and the
subsequent code runs into a NULL dereference on the port value, which
should have been looked up from cpkt->id.

Now, if we take a snapshot of cpkt, we naturally don't have this
problem, because we're looking at a consistent state of cpkt: it's
either PORT_ADD or PORT_REMOVE all the way. Which is what this patch
does.

Does this answer your question?

Thanks,
--
Alex

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Greg Kroah-Hartman]

On Fri, Jan 27, 2023 at 04:17:46PM +0200, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> >> >> We can have shared pages between the host and guest without bounce
> >> >> buffers in between, so they can be both looking directly at the same
> >> >> page.
> >> >> 
> >> >> Regards,
> >> >
> >> > How does this configuration work? What else is in this page?
> >> 
> >> So, for example in TDX, you have certain pages as "shared", as in
> >> between guest and hypervisor. You can have virtio ring(s) in such
> >> pages. It's likely that there'd be a swiotlb buffer there instead, but
> >> sharing pages between host virtio and guest virtio drivers is possible.
> >
> > If it is shared, then what does this mean?  Do we then need to copy
> > everything out of that buffer first before doing anything with it
> > because the data could change later on?  Or do we not trust anything in
> > it at all and we throw it away?  Or something else (trust for a short
> > while and then we don't?)
> 
> The first one, we need a consistent view of the metadata (the ckpt in
> this case), so we take a snapshot of it. Then, we validate it (because
> we don't trust it) to be correct. If it is not, we discard it, otherwise
> we act on it. Since this is a ring, we just move on to the next record
> if there is one.

So you do an additional extra copy of everything, making the bounce
buffer useless?  :)

> Meanwhile, in the shared page, it can change from correct to incorrect,
> but it won't affect us because we have this consistent view at the
> moment the snapshot was taken.

Wonderful, copy everything out then, the whole page, don't do it
piecemeal field by field.  And then justify it to everyone whose
throughput you just tanked...

good luck!

greg k-h

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Michael S. Tsirkin]

On Fri, Jan 27, 2023 at 04:17:46PM +0200, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> >> >> We can have shared pages between the host and guest without bounce
> >> >> buffers in between, so they can be both looking directly at the same
> >> >> page.
> >> >> 
> >> >> Regards,
> >> >
> >> > How does this configuration work? What else is in this page?
> >> 
> >> So, for example in TDX, you have certain pages as "shared", as in
> >> between guest and hypervisor. You can have virtio ring(s) in such
> >> pages. It's likely that there'd be a swiotlb buffer there instead, but
> >> sharing pages between host virtio and guest virtio drivers is possible.
> >
> > If it is shared, then what does this mean?  Do we then need to copy
> > everything out of that buffer first before doing anything with it
> > because the data could change later on?  Or do we not trust anything in
> > it at all and we throw it away?  Or something else (trust for a short
> > while and then we don't?)
> 
> The first one, we need a consistent view of the metadata (the ckpt in
> this case), so we take a snapshot of it. Then, we validate it (because
> we don't trust it) to be correct. If it is not, we discard it, otherwise
> we act on it. Since this is a ring, we just move on to the next record
> if there is one.
> 
> Meanwhile, in the shared page, it can change from correct to incorrect,
> but it won't affect us because we have this consistent view at the
> moment the snapshot was taken.
> 
> > Please be specific as to what you want to see happen here, and why.
> 
> For example, if we get a control message to add a port and
> cpkt->event==PORT_ADD, we skip validation of cpkt->id (port id), because
> we're intending to add a new one. At this point, the device can change
> cpkt->event to PORT_REMOVE, which does require a valid cpkt->id and the
> subsequent code runs into a NULL dereference on the port value, which
> should have been looked up from cpkt->id.
> 
> Now, if we take a snapshot of cpkt, we naturally don't have this
> problem, because we're looking at a consistent state of cpkt: it's
> either PORT_ADD or PORT_REMOVE all the way. Which is what this patch
> does.
> 
> Does this answer your question?
> 
> Thanks,
> --
> Alex


Not sure about Greg but it doesn't answer my question because either the
bad device has access to all memory at which point it's not clear why
is it changing cpkt->event and not e.g. stack. Or it's restricted to
only access memory when mapped through the DMA API. Which is not the
case here.

-- 
MST

RE: [PATCH v1 2/6] virtio console: Harden port adding

[Reshetova, Elena]

> On Fri, Jan 27, 2023 at 04:17:46PM +0200, Alexander Shishkin wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >
> > > On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
> > >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > >>
> > >> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> > >> >> We can have shared pages between the host and guest without bounce
> > >> >> buffers in between, so they can be both looking directly at the same
> > >> >> page.
> > >> >>
> > >> >> Regards,
> > >> >
> > >> > How does this configuration work? What else is in this page?
> > >>
> > >> So, for example in TDX, you have certain pages as "shared", as in
> > >> between guest and hypervisor. You can have virtio ring(s) in such
> > >> pages. It's likely that there'd be a swiotlb buffer there instead, but
> > >> sharing pages between host virtio and guest virtio drivers is possible.
> > >
> > > If it is shared, then what does this mean?  Do we then need to copy
> > > everything out of that buffer first before doing anything with it
> > > because the data could change later on?  Or do we not trust anything in
> > > it at all and we throw it away?  Or something else (trust for a short
> > > while and then we don't?)
> >
> > The first one, we need a consistent view of the metadata (the ckpt in
> > this case), so we take a snapshot of it. Then, we validate it (because
> > we don't trust it) to be correct. If it is not, we discard it, otherwise
> > we act on it. Since this is a ring, we just move on to the next record
> > if there is one.
> >
> > Meanwhile, in the shared page, it can change from correct to incorrect,
> > but it won't affect us because we have this consistent view at the
> > moment the snapshot was taken.
> >
> > > Please be specific as to what you want to see happen here, and why.
> >
> > For example, if we get a control message to add a port and
> > cpkt->event==PORT_ADD, we skip validation of cpkt->id (port id), because
> > we're intending to add a new one. At this point, the device can change
> > cpkt->event to PORT_REMOVE, which does require a valid cpkt->id and the
> > subsequent code runs into a NULL dereference on the port value, which
> > should have been looked up from cpkt->id.
> >
> > Now, if we take a snapshot of cpkt, we naturally don't have this
> > problem, because we're looking at a consistent state of cpkt: it's
> > either PORT_ADD or PORT_REMOVE all the way. Which is what this patch
> > does.
> >
> > Does this answer your question?
> >
> > Thanks,
> > --
> > Alex
> 
> 
> Not sure about Greg but it doesn't answer my question because either the
> bad device has access to all memory at which point it's not clear why
> is it changing cpkt->event and not e.g. stack. Or it's restricted to
> only access memory when mapped through the DMA API. Which is not the
> case here.

We do enforce virtio usage via DMA API only for TDX guest. Alex has a patch
queued for that also. 
But not sure if this addresses your concern here. 

Best Regards,
Elena.

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Michael S. Tsirkin]

On Fri, Jan 27, 2023 at 02:47:55PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Fri, Jan 27, 2023 at 01:55:43PM +0200, Alexander Shishkin wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Thu, Jan 19, 2023 at 10:13:18PM +0200, Alexander Shishkin wrote:
> >> >> 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.
> >> 
> >> We can be, see below.
> >> 
> >> >> 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.
> >> 
> >> We can have shared pages between the host and guest without bounce
> >> buffers in between, so they can be both looking directly at the same
> >> page.
> >> 
> >> Regards,
> >
> > How does this configuration work? What else is in this page?
> 
> So, for example in TDX, you have certain pages as "shared", as in
> between guest and hypervisor. You can have virtio ring(s) in such
> pages.

That one's marked as dma coherent.

> It's likely that there'd be a swiotlb buffer there instead, but
> sharing pages between host virtio and guest virtio drivers is possible.

It's not something console does though, does it?

> Apologies if the language is confusing, I hope I'm answering the
> question.
> 
> Regards,
> --
> Alex

I'd like an answer to when does the console driver share the buffer
in question, not when generally some pages shared.

-- 
MST

Re: [PATCH v1 2/6] virtio console: Harden port adding

[Michael S. Tsirkin]

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>


I suspect anyone worried about this kind of thing already uses a bounce
buffer. No?  The patch itself makes the code more readable, except maybe
for the READ_ONCE thing.


> ---
>  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));
> +	port = find_port_by_id(portdev, id);
>  	if (!port &&
>  	    cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) {
>  		/* No valid header at start of buffer.  Drop it. */
> @@ -1583,15 +1586,14 @@ 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) >=
> -		    portdev->max_nr_ports) {
> +		if (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);
>  			break;
>  		}
> -		add_port(portdev, virtio32_to_cpu(vdev, cpkt->id));
> +		add_port(portdev, id);
>  		break;
>  	case VIRTIO_CONSOLE_PORT_REMOVE:
>  		unplug_port(port);
> -- 
> 2.39.0

[PATCH v1 3/6] virtio 9p: Fix an overflow

[Alexander Shishkin]

From: Andi Kleen <ak@linux.intel.com>

tag_len is read as a u16 from the untrusted host. It could overflow
in the memory allocation, which would lead to a too small buffer.

Some later loops use it when extended to 32bit, so they could overflow
the too small buffer.

Make sure to do the arithmetic for the buffer size in 32bit to avoid
wrapping.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Latchesar Ionkov <lucho@ionkov.net>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: v9fs-developer@lists.sourceforge.net
---
 net/9p/trans_virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 3c27ffb781e3..a78e4d80e5ba 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -629,7 +629,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		err = -EINVAL;
 		goto out_free_vq;
 	}
-	tag = kzalloc(tag_len + 1, GFP_KERNEL);
+	tag = kzalloc((u32)tag_len + 1, GFP_KERNEL);
 	if (!tag) {
 		err = -ENOMEM;
 		goto out_free_vq;
-- 
2.39.0

Re: [PATCH v1 3/6] virtio 9p: Fix an overflow

[Michael S. Tsirkin]

On Thu, Jan 19, 2023 at 03:57:18PM +0200, Alexander Shishkin wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> tag_len is read as a u16 from the untrusted host. It could overflow
> in the memory allocation, which would lead to a too small buffer.
> 
> Some later loops use it when extended to 32bit, so they could overflow
> the too small buffer.
> 
> Make sure to do the arithmetic for the buffer size in 32bit to avoid
> wrapping.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> Cc: Eric Van Hensbergen <ericvh@gmail.com>
> Cc: Latchesar Ionkov <lucho@ionkov.net>
> Cc: Dominique Martinet <asmadeus@codewreck.org>
> Cc: v9fs-developer@lists.sourceforge.net
> ---
>  net/9p/trans_virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 3c27ffb781e3..a78e4d80e5ba 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -629,7 +629,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
>  		err = -EINVAL;
>  		goto out_free_vq;
>  	}
> -	tag = kzalloc(tag_len + 1, GFP_KERNEL);
> +	tag = kzalloc((u32)tag_len + 1, GFP_KERNEL);
>  	if (!tag) {
>  		err = -ENOMEM;
>  		goto out_free_vq;

Hmm are you sure there's a difference in behaviour? I thought C will just
extend the integer to int.

> -- 
> 2.39.0

Re: [PATCH v1 3/6] virtio 9p: Fix an overflow

[Alexander Shishkin]

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

> On Thu, Jan 19, 2023 at 03:57:18PM +0200, Alexander Shishkin wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>> 
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index 3c27ffb781e3..a78e4d80e5ba 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -629,7 +629,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
>>  		err = -EINVAL;
>>  		goto out_free_vq;
>>  	}
>> -	tag = kzalloc(tag_len + 1, GFP_KERNEL);
>> +	tag = kzalloc((u32)tag_len + 1, GFP_KERNEL);
>>  	if (!tag) {
>>  		err = -ENOMEM;
>>  		goto out_free_vq;
>
> Hmm are you sure there's a difference in behaviour? I thought C will just
> extend the integer to int.

Actually, you're right, integer promotion would extend the original
expression to int. I'll drop this patch also.

Thanks,
--
Alex

[PATCH v1 4/6] virtio console: Harden control message handling

[Alexander Shishkin]

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

Re: [PATCH v1 4/6] virtio console: Harden control message handling

[Greg Kroah-Hartman]

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

Re: [PATCH v1 4/6] virtio console: Harden control message handling

[Michael S. Tsirkin]

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

Re: [PATCH v1 4/6] virtio console: Harden control message handling

[Alexander Shishkin]

"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

Re: [PATCH v1 4/6] virtio console: Harden control message handling

[Michael S. Tsirkin]

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

Re: [PATCH v1 4/6] virtio console: Harden control message handling

[Alexander Shishkin]

"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

[PATCH v1 5/6] virtio_net: Guard against buffer length overflow in xdp_linearize_page()

[Alexander Shishkin]

When reassembling incoming buffers to an xdp_page, there is a potential
integer overflow in the buffer size test and trigger and out of bounds
memcpy().

Fix this by reordering the test so that both sides are of the same
signedness.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jesper Dangaard Brouer <hawk@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7723b2a49d8e..dfa51dd95f63 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -751,8 +751,10 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
 
 		/* guard against a misconfigured or uncooperative backend that
 		 * is sending packet larger than the MTU.
+		 * At the same time, make sure that an especially uncooperative
+		 * backend can't overflow the test by supplying a large buflen.
 		 */
-		if ((page_off + buflen + tailroom) > PAGE_SIZE) {
+		if (buflen > PAGE_SIZE - page_off - tailroom) {
 			put_page(p);
 			goto err_buf;
 		}
-- 
2.39.0

Re: [PATCH v1 5/6] virtio_net: Guard against buffer length overflow in xdp_linearize_page()

[Michael S. Tsirkin]

On Thu, Jan 19, 2023 at 03:57:20PM +0200, Alexander Shishkin wrote:
> When reassembling incoming buffers to an xdp_page, there is a potential
> integer overflow in the buffer size test and trigger and out of bounds
> memcpy().
> 
> Fix this by reordering the test so that both sides are of the same
> signedness.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jesper Dangaard Brouer <hawk@kernel.org>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/virtio_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7723b2a49d8e..dfa51dd95f63 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -751,8 +751,10 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
>  
>  		/* guard against a misconfigured or uncooperative backend that
>  		 * is sending packet larger than the MTU.
> +		 * At the same time, make sure that an especially uncooperative
> +		 * backend can't overflow the test by supplying a large buflen.
>  		 */
> -		if ((page_off + buflen + tailroom) > PAGE_SIZE) {
> +		if (buflen > PAGE_SIZE - page_off - tailroom) {
>  			put_page(p);
>  			goto err_buf;
>  		}

I feel the issue should be addressed in virtqueue_get_buf.
In fact, when using DMA API, we already keep track of the
length in vring_desc_extra.

So, isn't this just the question of passing the length and
validating it e.g. in vring_unmap_one_split?
We can also use the index_nospec trick since otherwise
there could be speculation concerns.

> -- 
> 2.39.0

[PATCH v1 6/6] virtio_ring: Prevent bounds check bypass on descriptor index

[Alexander Shishkin]

The descriptor index in virtqueue_get_buf_ctx_split() comes from the
device/VMM.a Use array_index_nospec() to prevent the CPU from speculating
beyond the descriptor array bounds and providing a primitive for building
a side channel.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/virtio/virtio_ring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 2e7689bb933b..c42d070ab68d 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/nospec.h>
 #include <linux/hrtimer.h>
 #include <linux/dma-mapping.h>
 #include <linux/kmsan.h>
@@ -819,6 +820,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
 		BAD_RING(vq, "id %u out of range\n", i);
 		return NULL;
 	}
+
+	i = array_index_nospec(i, vq->split.vring.num);
 	if (unlikely(!vq->split.desc_state[i].data)) {
 		BAD_RING(vq, "id %u is not a head!\n", i);
 		return NULL;
-- 
2.39.0

Re: [PATCH v1 6/6] virtio_ring: Prevent bounds check bypass on descriptor index

[Michael S. Tsirkin]

On Thu, Jan 19, 2023 at 03:57:21PM +0200, Alexander Shishkin wrote:
> The descriptor index in virtqueue_get_buf_ctx_split() comes from the
> device/VMM.a Use array_index_nospec() to prevent the CPU from speculating
> beyond the descriptor array bounds and providing a primitive for building
> a side channel.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/virtio/virtio_ring.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 2e7689bb933b..c42d070ab68d 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -9,6 +9,7 @@
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/nospec.h>
>  #include <linux/hrtimer.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/kmsan.h>
> @@ -819,6 +820,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>  		BAD_RING(vq, "id %u out of range\n", i);
>  		return NULL;
>  	}
> +
> +	i = array_index_nospec(i, vq->split.vring.num);

I suspect plain
	 i &= split.vring.num - 1
is more efficient.

We know num is a power of two but compiler doesn't.
And pls add a comment explaining what's going on.

>  	if (unlikely(!vq->split.desc_state[i].data)) {
>  		BAD_RING(vq, "id %u is not a head!\n", i);
>  		return NULL;
> -- 
> 2.39.0

Re: [PATCH v1 0/6] Harden a few virtio bits

[Michael S. Tsirkin]

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

Re: [PATCH v1 0/6] Harden a few virtio bits

[Alexander Shishkin]

"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

Re: [PATCH v1 0/6] Harden a few virtio bits

[Michael S. Tsirkin]

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