Re: [syzbot] KASAN: vmalloc-out-of-bounds Write in imageblit (2)

[Khalid Masum <khalid.masum.92@gmail.com>]

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