Messages in this thread | | | Date | Sun, 31 Jul 2022 17:23:21 +0600 | Subject | Re: [syzbot] KASAN: vmalloc-out-of-bounds Write in imageblit (2) | From | Khalid Masum <> |
| |
On 7/31/22 16:54, Helge Deller wrote: > * Khalid Masum <khalid.masum.92@gmail.com>: >> 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
| |