On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
> From: Andi Kleen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Alexander Shishkin <[email protected]>
> Cc: Amit Shah <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
> 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 <[email protected]> writes:
> On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
>> From: Andi Kleen <[email protected]>
>>
>> --- 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 <[email protected]> writes:
>
> > On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
> >> From: Andi Kleen <[email protected]>
> >>
> >> --- 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