diff mbox series

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

Message ID 20230119135721.83345-2-alexander.shishkin@linux.intel.com
State New
Headers
Series Harden a few virtio bits |

Commit Message

Alexander Shishkin Jan. 19, 2023, 1:57 p.m. UTC 
  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(+)

Comments

Greg KH Jan. 19, 2023, 7:18 p.m. UTC | #1 
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
Michael S. Tsirkin Jan. 20, 2023, 1:01 p.m. UTC | #2 
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
Alexander Shishkin Jan. 20, 2023, 3:51 p.m. UTC | #3 
"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
diff mbox series

Patch

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