KASAN: vmalloc-out-of-bounds Write in imageblit (2)

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

The line and column numbers for the selection need to start at 1. Add the checks to prevent invalid input.

Signed-off-by Helge Deller <[email protected]>

Reported-by [email protected]

drivers/tty/vt/selection.c +0 -0

index f7755e73696e..58692a9b4097 100644

Quoted 14 lines · Helge Deller

The line and column numbers for the selection need to start at 1. Add the checks to prevent invalid input.

Signed-off-by Helge Deller <[email protected]>

Reported-by [email protected]

drivers/tty/vt/selection.c +0 -0

index f7755e73696e..58692a9b4097 100644
@@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
           return 0;
       }
 

drivers/tty/vt/selection.c static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, · line 330

Quoted 2 lines

drivers/tty/vt/selection.c +2 -0

+    if (!v->xs || !v->ys || !v->xe || !v->ye)
+        return -EINVAL;

Hmm, I'm not sure about this. It potentially breaks userspace (by returning EINVAL now).

And the code below should handle this just fine, right:

Quoted 3 lines

drivers/tty/vt/selection.c +0 -0

      v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
      v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
      v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);

Hello Jiri,

Thanks for looking into this patch!

Quoted 15 lines · Jiri Slaby

Quoted 14 lines · Helge Deller

The line and column numbers for the selection need to start at 1. Add the checks to prevent invalid input.

Signed-off-by Helge Deller <[email protected]>

Reported-by [email protected]

drivers/tty/vt/selection.c +0 -0

index f7755e73696e..58692a9b4097 100644
@@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
           return 0;
       }
 

Quoted 5 lines

drivers/tty/vt/selection.c static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, · line 330

Quoted 2 lines

drivers/tty/vt/selection.c +2 -0

+    if (!v->xs || !v->ys || !v->xe || !v->ye)
+        return -EINVAL;

Hmm, I'm not sure about this. It potentially breaks userspace (by returning EINVAL now).

Right.
According to the code below, my interpretation is that all xs/ys/xe/ye values should be > 0. But of course I might be wrong on this, as I didn't find any documentation for TIOCL_SETSEL.

And if userspace tries to set an invalid selection (e.g. by selecting row 0), my patch now returns -EINVAL, while it returned success before.

Quoted 4 lines

And the code below should handle this just fine, right:

Quoted 3 lines

drivers/tty/vt/selection.c +0 -0

      v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
      v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
      v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);

It "handles it fine" in the sense that it can cope with the input and will not crash.
But it returns (maybe?) unexpected results...

Quoted 34 lines · Helge Deller

Hello Jiri,

Thanks for looking into this patch!

Quoted 15 lines · Jiri Slaby

Quoted 14 lines · Helge Deller

The line and column numbers for the selection need to start at 1. Add the checks to prevent invalid input.

Signed-off-by Helge Deller <[email protected]>

Reported-by [email protected]

drivers/tty/vt/selection.c +0 -0

index f7755e73696e..58692a9b4097 100644
@@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
           return 0;
       }
 

Quoted 5 lines

drivers/tty/vt/selection.c static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, · line 330

Quoted 2 lines

drivers/tty/vt/selection.c +2 -0

+    if (!v->xs || !v->ys || !v->xe || !v->ye)
+        return -EINVAL;

Hmm, I'm not sure about this. It potentially breaks userspace (by returning EINVAL now).

Right.
According to the code below, my interpretation is that all xs/ys/xe/ye values should be > 0. But of course I might be wrong on this, as I didn't find any documentation for TIOCL_SETSEL.

And if userspace tries to set an invalid selection (e.g. by selecting row 0), my patch now returns -EINVAL, while it returned success before.

Quoted 8 lines

Quoted 4 lines

And the code below should handle this just fine, right:

Quoted 3 lines

drivers/tty/vt/selection.c +0 -0

       v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
       v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
       v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);

It "handles it fine" in the sense that it can cope with the input and will not crash.
But it returns (maybe?) unexpected results...

After some more thinking maybe you are right.
In case a user provided invalid values in the past, simply an unexpected selection was set, but nothing broke.
Since the patch doesn't fix any critical issue, we could just drop this patch and leave it as is.

Quoted 6 lines · Helge Deller

Quoted 4 lines · Helge Deller

Quoted 3 lines · Jiri Slaby

Quoted 2 lines · Helge Deller

The line and column numbers for the selection need to start at 1. Add the checks to prevent invalid input.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/tty/vt/selection.c static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, · line 327

Quoted 5 lines

drivers/tty/vt/selection.c +2 -0

@@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
+    if (!v->xs || !v->ys || !v->xe || !v->ye)
+        return -EINVAL;

Hmm, I'm not sure about this. It potentially breaks userspace (by returning EINVAL now).

We can still do a trial and revert it if something breaks... It's just that _noone_ knows with all this undocumented stuff ;).

But in fact, 0 currently means full row/column. Isn't it on purpose?

Today, we are out of luck, codesearch.debian.net gives no clue about users:
https://codesearch.debian.net/search?q=%5CbTIOCL_SETSEL%5Cb&literal=0

When changing the console font with ioctl(KDFONTOP) the new font size can be bigger than the previous font. A previous selection may thus now be outside of the new screen size and thus trigger out-of-bounds accesses to graphics memory if the selection is removed in vc_do_resize().

Prevent such out-of-memory accesses by dropping the selection before the various con_font_set() console handlers are called.

Signed-off-by Helge Deller <[email protected]>

Reported-by [email protected]

drivers/tty/vt/vt.c +8 -4

index dfc1f4b445f3..3f09205185a4 100644
@@ -4662,9 +4662,11 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
  	console_lock();
  	if (vc->vc_mode != KD_TEXT)
  		rc = -EINVAL;
-	else if (vc->vc_sw->con_font_set)
+	else if (vc->vc_sw->con_font_set) {
+		if (vc_is_sel(vc))
+			clear_selection();
  		rc = vc->vc_sw->con_font_set(vc, &font, op->flags);
-	else
+	} else
  		rc = -ENOSYS;
  	console_unlock();
  	kfree(font.data);

@@ -4691,9 +4693,11 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
  		console_unlock();
  		return -EINVAL;
  	}
-	if (vc->vc_sw->con_font_default)
+	if (vc->vc_sw->con_font_default) {
+		if (vc_is_sel(vc))
+			clear_selection();
  		rc = vc->vc_sw->con_font_default(vc, &font, s);
-	else
+	} else
  		rc = -ENOSYS;
  	console_unlock();
  	if (!rc) {

Quoted 3 lines · Khalid Masum

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

The reproducer does this:

ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0

-> sets the text selection area
ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0

-> changes the font size.

It does not crash with current Linus' head (v5.19-rc8).

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):

  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.

Helge

Quoted 5 lines · Khalid Masum

Quoted 3 lines

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

Quoted 1 line

The reproducer does this:

Thanks for Looking into this. Being to this, so I have some questions.

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 17 lines

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.

Quoted 6 lines

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 18 lines

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

Quoted 1 line

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.

* Khalid Masum <[email protected]>:

Quoted 6 lines · Helge Deller

Quoted 5 lines · Khalid Masum

Quoted 3 lines

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

Quoted 7 lines

Quoted 1 line

The reproducer does this:

Thanks for Looking into this. Being to this, so I have some questions.

Quoted 1 line

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 4 lines

Quoted 2 lines

-> 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 5 lines

Quoted 3 lines

-> 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 44 lines

Quoted 17 lines

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.

Quoted 6 lines

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 18 lines

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

Quoted 3 lines

Quoted 1 line

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?

Below is my debug code.

Helge

drivers/tty/vt/selection.c +2 -0

index 58692a9b4097..0167b368a70f 100644
@@ -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);

drivers/tty/vt/vt.c +4 -0

index 3f09205185a4..a0b4570c959a 100644
@@ -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;

Quoted 3 lines · Khalid Masum

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

The reproducer does this:
ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0 -> sets the text selection area
ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0 -> changes the font size.

It does not crash with current Linus' head (v5.19-rc8).
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):

  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.

Helge

Quoted 16 lines · Helge Deller

Quoted 3 lines · Khalid Masum

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

The reproducer does this:
ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3  ys:0  xe:0 ye:0 mode:0)  = 0 -> sets the text selection area
ioctl(4, KDFONTOP)  with op=0 (con_font_set), charcount=512  width=8  height=32, 0x20000000) = 0 -> changes the font size.

It does not crash with current Linus' head (v5.19-rc8).
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.

Quoted 4 lines

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.

Why do you think that API expects a 1?

Quoted 17 lines · Khalid Masum

Quoted 16 lines · Helge Deller

Quoted 3 lines · Khalid Masum

Here is a simplified reproducer for the issue:

https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c

The reproducer does this:
ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3  ys:0  xe:0 ye:0 mode:0)  = 0 -> sets the text selection area
ioctl(4, KDFONTOP)  with op=0 (con_font_set), charcount=512  width=8  height=32, 0x20000000) = 0 -> changes the font size.

It does not crash with current Linus' head (v5.19-rc8).
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.

Quoted 6 lines

Quoted 4 lines

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.

Why do you think that API expects a 1?

because the code in drivers/tty/vt/selection.c indicates this:
See:
static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, struct tty_struct *tty)

{
         int ps, pe;
...
         v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
         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);
...
         ps = v->ys * vc->vc_size_row + (v->xs << 1);
         pe = v->ye * vc->vc_size_row + (v->xe << 1);

The userspace-provided xs,ys,xe and ye values are decremented by one before ps and pe (the pointers into the text screen array) are calculated. So, for the xs...ye values in the range from 1..max-screen-lines/columns are expected.

Helge

Currently the if block's condition has an unhandled case, where the result of ret might get greater than vc->vc_scr_end, and therefore the corresponding handler in else block never gets executed. Which eventually causes panic in fast_imageblit.

Add this extra check in the conditions to fix this breakage.

#syz-test: https://github.com/torvalds/linux.git e0dccc3b76fb

---
 drivers/video/fbdev/core/fbcon.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

drivers/video/fbdev/core/fbcon.c +4 -0

index 1a9aa12cf886..d026f3845b60 100644
@@ -2591,14 +2591,13 @@ static unsigned long fbcon_getxy(struct vc_data *vc, unsigned long pos,
 {
 	unsigned long ret;
 	int x, y;
+	unsigned long offset = (pos - vc->vc_origin) / 2;
+	x = offset % vc->vc_cols;
+	y = offset / vc->vc_cols;
+	ret = pos + (vc->vc_cols - x) * 2;

These are precendence bugs. The ! will be done before the >=. Write it as:

	if (pos < vc->vc_origin || pos >= vc->vc_scr_end ||
	    ret >= vc->vc_scr_end) {

Quoted 3 lines

  		/* Should not happen */
  		x = y = 0;
  		ret = vc->vc_origin;

regards,
dan carpenter