[v1] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

[PATCH] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

Posted by Albin Babu Varghese 4 months, 1 week ago

KASAN reports vmalloc-out-of-bounds writes in sys_imageblit during console
resize operations. The crash happens when bit_putcs renders characters 
outside the allocated framebuffer region.

Call trace: vc_do_resize -> clear_selection -> invert_screen ->
do_update_region -> fbcon_putcs -> bit_putcs -> sys_imageblit

The console resize changes dimensions but bit_putcs doesn't validate that 
the character positions fit within the framebuffer before rendering. 
This causes writes past the allocated buffer in fb_imageblit functions.

Fix by checking bounds before rendering:
- Return if dy + height > yres (would write past bottom)
- Break if dx + width > xres (would write past right edge)

Reported-by: syzbot+48b0652a95834717f190@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=48b0652a95834717f190  
Tested-by: syzbot+48b0652a95834717f190@syzkaller.appspotmail.com
Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@gmail.com>
---
 drivers/video/fbdev/core/bitblit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index f9475c14f733..4c732284384a 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -160,6 +160,9 @@ static void bit_putcs(struct vc_data *vc, struct fb_info *info,
 	image.height = vc->vc_font.height;
 	image.depth = 1;
 
+	if (image.dy + image.height > info->var.yres)
+		return;
+
 	if (attribute) {
 		buf = kmalloc(cellsize, GFP_ATOMIC);
 		if (!buf)
@@ -173,6 +176,10 @@ static void bit_putcs(struct vc_data *vc, struct fb_info *info,
 			cnt = count;
 
 		image.width = vc->vc_font.width * cnt;
+
+		if (image.dx + image.width > info->var.xres)
+			break;
+
 		pitch = DIV_ROUND_UP(image.width, 8) + scan_align;
 		pitch &= ~scan_align;
 		size = pitch * image.height + buf_align;
-- 
2.51.0

Re: [PATCH] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

Posted by Helge Deller 4 months, 1 week ago

On 9/27/25 09:50, Albin Babu Varghese wrote:
> KASAN reports vmalloc-out-of-bounds writes in sys_imageblit during console
> resize operations. The crash happens when bit_putcs renders characters
> outside the allocated framebuffer region.
> 
> Call trace: vc_do_resize -> clear_selection -> invert_screen ->
> do_update_region -> fbcon_putcs -> bit_putcs -> sys_imageblit
> 
> The console resize changes dimensions but bit_putcs doesn't validate that
> the character positions fit within the framebuffer before rendering.
> This causes writes past the allocated buffer in fb_imageblit functions.
> 
> Fix by checking bounds before rendering:
> - Return if dy + height > yres (would write past bottom)
> - Break if dx + width > xres (would write past right edge)
> 
> Reported-by: syzbot+48b0652a95834717f190@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=48b0652a95834717f190
> Tested-by: syzbot+48b0652a95834717f190@syzkaller.appspotmail.com
> Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@gmail.com>
> ---
>   drivers/video/fbdev/core/bitblit.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index f9475c14f733..4c732284384a 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -160,6 +160,9 @@ static void bit_putcs(struct vc_data *vc, struct fb_info *info,
>   	image.height = vc->vc_font.height;
>   	image.depth = 1;
>   
> +	if (image.dy + image.height > info->var.yres)
> +		return;
> +

I wonder if the image.height value should be capped in this case,
instead of not rendering any chars at all?
Something like (untested!):

+	if (image.dy >= info->var.yres)
+		return;
+       image.height = min(image.height, info->var.yres - image.dy);


>   	if (attribute) {
>   		buf = kmalloc(cellsize, GFP_ATOMIC);
>   		if (!buf)
> @@ -173,6 +176,10 @@ static void bit_putcs(struct vc_data *vc, struct fb_info *info,
>   			cnt = count;
>   
>   		image.width = vc->vc_font.width * cnt;
> +
> +		if (image.dx + image.width > info->var.xres)
> +			break;
> +

same here.


>   		pitch = DIV_ROUND_UP(image.width, 8) + scan_align;
>   		pitch &= ~scan_align;
>   		size = pitch * image.height + buf_align;

Re: [PATCH] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

Posted by Albin Babu Varghese 4 months, 1 week ago

Hi Helge, Thanks for the review.

> I wonder if the image.height value should be capped in this case,
> instead of not rendering any chars at all?
> Something like (untested!):
> 
> +	if (image.dy >= info->var.yres)
> +		return;
> +       image.height = min(image.height, info->var.yres - image.dy);
 
This looks like a better implementation than what I had. I thought it might be
better to skip the entire row instead of rendering partially. I’m still new to
this subsystem, so thanks for pointing this out. I’ll test the suggested
changes and send a v2.

Re: [PATCH] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

Posted by Helge Deller 4 months, 1 week ago

On 10/1/25 19:19, Albin Babu Varghese wrote:
> Hi Helge, Thanks for the review.
> 
>> I wonder if the image.height value should be capped in this case,
>> instead of not rendering any chars at all?
>> Something like (untested!):
>>
>> +	if (image.dy >= info->var.yres)
>> +		return;
>> +       image.height = min(image.height, info->var.yres - image.dy);
>   
> This looks like a better implementation than what I had. 

I just added comments - not sure if mine was better.,

> I thought it might be better to skip the entire row instead of
> rendering partially.

Do you know if this affects the selection?
If so, would modifying (reducing/shortening) the selection maybe fix it?

> I’m still new to this subsystem, so thanks for pointing this out.
> I’ll test the suggested changes and send a v2.
Thanks for testing and checking!

Helge

Re: [PATCH] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

Posted by Albin Babu Varghese 4 months, 1 week ago

Hi Helge, I tested your suggestions and they seem to work well.

> Do you know if this affects the selection?
> If so, would modifying (reducing/shortening) the selection maybe fix it?

The syzkaller reproducer uses really weird values where xs > xe and ys > ye
(xs=0xa00, xe=0x101, ys=0xc7e, ye=0x100) and set_selection() already swaps them
if needed and clamps the values.

I added debug prints to check what's happening and the clamping in
set_selection() is working and the values coming through are within bounds. But
the crash still happens when you remap the framebuffer because of a slight
overflow.

I also discovered that when image.width is clipped on the X-axis, the character
count (cnt) must also be updated to match, otherwise bit_putcs_aligned()
	receives mismatched buffer size and character count parameters, causing
	out-of-bounds writes.

So I changed it to something like this:

+	if (image.dx >= info->var.xres)
+		break;
+	if (image.dx + image.width > info->var.xres) {
+		image.width = info->var.xres - image.dx;
+		cnt = image.width / vc->vc_font.width;
+		if (cnt == 0)
+			break;
+		image.width = cnt * vc->vc_font.width;
+	}

I tested it in syzbot, with the syzkaller reproducer, and also manually in QEMU
and verified that the buffer switches from tty1 to tty2 work correctly.

I couldn’t find a dedicated fbdev/fbcon test suite. Beyond kselftests, do you
recommend anything specific before sending v2?

Thanks,
Albin

Re: [PATCH] fbdev: Add bounds checking in bit_putcs to fix vmalloc-out-of-bounds

Posted by Helge Deller 4 months, 1 week ago

On 10/2/25 10:52, Albin Babu Varghese wrote:
> Hi Helge, I tested your suggestions and they seem to work well.
> 
>> Do you know if this affects the selection?
>> If so, would modifying (reducing/shortening) the selection maybe fix it?
> 
> The syzkaller reproducer uses really weird values where xs > xe and ys > ye
> (xs=0xa00, xe=0x101, ys=0xc7e, ye=0x100) and set_selection() already swaps them
> if needed and clamps the values.

Ok.
  
> I added debug prints to check what's happening and the clamping in
> set_selection() is working and the values coming through are within bounds. But
> the crash still happens when you remap the framebuffer because of a slight
> overflow.
> 
> I also discovered that when image.width is clipped on the X-axis, the character
> count (cnt) must also be updated to match, otherwise bit_putcs_aligned()
> 	receives mismatched buffer size and character count parameters, causing
> 	out-of-bounds writes.
> 
> So I changed it to something like this:
> 
> +	if (image.dx >= info->var.xres)
> +		break;
> +	if (image.dx + image.width > info->var.xres) {
> +		image.width = info->var.xres - image.dx;
> +		cnt = image.width / vc->vc_font.width;
> +		if (cnt == 0)
> +			break;
> +		image.width = cnt * vc->vc_font.width;
> +	}

Looks good.

> I tested it in syzbot, with the syzkaller reproducer, and also manually in QEMU
> and verified that the buffer switches from tty1 to tty2 work correctly.
> 
> I couldn’t find a dedicated fbdev/fbcon test suite. Beyond kselftests, do you
> recommend anything specific before sending v2?

There is Geert's fbdev test tool (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git/),
but it does not involve testing of fbcon.

Helge