Re: [syzbot] KASAN: vmalloc-out-of-bounds Write in imageblit (2)
From: Khalid Masum
Date: Sun Jul 31 2022 - 07:23:35 EST
On 7/31/22 16:54, Helge Deller wrote:
* Khalid Masum <khalid.masum.92@xxxxxxxxx>:
On 7/30/22 23:25, Helge Deller wrote:
On 7/29/22 08:51, Khalid Masum wrote:
Here is a simplified reproducer for the issue:
https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
The reproducer does this:
Thanks for Looking into this. Being to this, so I have some questions.
ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
How did you find out the selection values? From strace and man pages I know
the third argument is an address.
Right. It's a pointer into userspace.
I simply added printk debug code to see what's happening.
I've attached that patch below.
-> sets the text selection area
ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
Same here, It would be very helpful if you could tell me how.
See patch below.
-> changes the font size.
It does not crash with current Linus' head (v5.19-rc8).
I tested in 5.19-rc8 in Qemu x86_64 and it crashed for me.
That's strange, since I tested the same. Maybe I did something wrong.
Anyway, the patches I sent applies to all kernel versions.
Kernel v5.16, which was used by this KASAN report, hasn't received backports
since months, so I tried stable kernel v5.15.58 instead, and this
kernel crashed with the reproducer.
The reproducer brings up two issues with current code:
1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
zero-values for ys and ye for the starting lines.
This is wrong, since the API seems to expect a "1" as the very first line for the selection.
This can be easily fixed by adding checks for zero-values and return -EINVAL if found.
But this bug isn't critical itself and is not the reason for the kernel crash.
Without the checks, the ioctl handler simply wraps the coordinate values and converts them
from:
input selection: xs:3 ys:0 xe:0 ye:0 mode:0 to the new:
vc_selection = xs:2 ys:23 xe:127 ye:23 mode:0
which is the current maximum coordinates for the screen.
Those higher values now trigger issue #2:
After the TIOCL_SETSEL the last line on the screen is now selected. The KDFONTOP ioctl
then sets a 8x32 console font, and replaces the former 8x16 console font.
With the bigger font the current screen selection is now outside the visible screen
and this finally triggeres this backtrace, because vc_do_resize() calls clear_selection()
to unhighlight the selection (which starts to render chars outside of the screen):
That makes sense.
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
highlight drivers/tty/vt/selection.c:57 [inline]
clear_selection drivers/tty/vt/selection.c:84 [inline]
clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
IMHO the easiest way to prevent this crash is to simply clear the
selection before the various con_font_set() console handlers are called.
Otherwise every console driver needs to add checks and verify if the current
selection still fits with the selected font, which gets tricky because some
of those drivers fiddle with the screen width&height before calling vc_do_resize().
I'll follow up to this mail with patches for both issues shortly.
I tested the patches. The crash no longer occurs with the reproducer.
Thanks for testing!
Maybe you want to reply to the patches with a Tested-by: tag?
Sure, I will do that.
Below is my debug code.
Thanks for the debug code! It explains a lot.
Helge
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 58692a9b4097..0167b368a70f 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -333,6 +333,7 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+ printk("vc_selection = xs:%u ys:%u xe:%u ye:%u mode:%u\n", v->xs, v->ys, v->xe, v->ye, v->sel_mode);
if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
@@ -357,6 +358,7 @@ int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
{
int ret;
+ printk("tiocl_selection = xs:%u ys:%u xe:%u ye:%u mode:%u\n", v->xs, v->ys, v->xe, v->ye, v->sel_mode);
mutex_lock(&vc_sel.lock);
console_lock();
ret = vc_selection(vc_cons[fg_console].d, v, tty);
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 3f09205185a4..a0b4570c959a 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3194,6 +3194,8 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
return -EFAULT;
ret = 0;
+ printk("tioclinux: type = %d\n", type); // TIOCL_SETSEL
+
switch (type)
{
case TIOCL_SETSEL:
@@ -4655,6 +4657,8 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
if (IS_ERR(font.data))
return PTR_ERR(font.data);
+ pr_err("con_font_set charcount %d w:%d h:%d\n", op->charcount, op->width, op->height);
+
font.charcount = op->charcount;
font.width = op->width;
font.height = op->height;
@@ -4709,6 +4713,7 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
int con_font_op(struct vc_data *vc, struct console_font_op *op)
{
+ pr_warn("con_font_op op = %d\n", op->op);
switch (op->op) {
case KD_FONT_OP_SET:
return con_font_set(vc, op);
Thanks,
-- Khalid Masum