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.0On 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
"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
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
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 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
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
© 2016 - 2025 Red Hat, Inc.