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

On 7/31/22 16:54, Helge Deller wrote:

* Khalid Masum [off-list ref]:

quoted

On 7/30/22 23:25, Helge Deller wrote:

quoted

On 7/29/22 08:51, Khalid Masum wrote:

quoted

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.

quoted

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.

quoted

quoted

-> 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.

quoted

quoted

-> 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.

quoted

quoted

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.

quoted

   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.

Thanks,
  -- Khalid Masum