[PATCH] fbdev: Delay the setting of fbcon_ops to fix KASAN issues

[PATCH] fbdev: Delay the setting of fbcon_ops to fix KASAN issues

[Zizhi Wo]

[BUG]
Recently, we encountered a KASAN warning as follows:

kasan_report+0xaf/0xe0 mm/kasan/report.c:588
fb_pad_aligned_buffer+0x12f/0x150 drivers/video/fbdev/core/fbmem.c:116
ccw_putcs_aligned drivers/video/fbdev/core/fbcon_ccw.c:119 [inline]
ccw_putcs+0x9ac/0xbb0 drivers/video/fbdev/core/fbcon_ccw.c:175
fbcon_putcs+0x329/0x3f0 drivers/video/fbdev/core/fbcon.c:1297
do_update_region+0x3de/0x670 drivers/tty/vt/vt.c:623
invert_screen+0x1de/0x600 drivers/tty/vt/vt.c:748
highlight drivers/tty/vt/selection.c:57 [inline]
clear_selection+0x5e/0x70 drivers/tty/vt/selection.c:81
vc_do_resize+0xc8e/0xf40 drivers/tty/vt/vt.c:1206
fbcon_modechanged+0x489/0x7a0 drivers/video/fbdev/core/fbcon.c:2705
fbcon_set_all_vcs+0x1e0/0x600 drivers/video/fbdev/core/fbcon.c:2752
fbcon_rotate_all drivers/video/fbdev/core/fbcon.c:250 [inline]
...

reproduce[probabilistic, depending on the width and height of vc_font, as
well as the value of "p" in do_update_region()]:
1) echo 2 > /sys/devices/virtual/graphics/fbcon/rotate_all
2) echo 3 > /sys/devices/virtual/graphics/fbcon/rotate_all

[CAUSE]
The root cause is that fbcon_modechanged() first sets the current rotate's
corresponding ops. Subsequently, during vc_resize(), it may trigger
clear_selection(), and in fbcon_putcs->ccw_putcs[rotate=3], this can result
in an out-of-bounds access to "src". This happens because ops->fontbuffer
is reallocated in fbcon_rotate_font():
1) When rotate=2, its size is (width + 7) / 8 * height
2) When rotate=3, its size is (height + 7) / 8 * width

And the call to fbcon_rotate_font() occurs after clear_selection(). In
other words, the fontbuffer is allocated using the size calculated from the
previous rotation[2], but before reallocating it with the new size,
con_putcs is already using the new rotation[3]:

rotate_all_store
 fbcon_rotate_all
  fbcon_set_all_vcs
   fbcon_modechanged
   ...
    fbcon_set_rotate
     fbcon_rotate_ccw
      ops->putcs = ccw_putcs // set rotate 3 ops
    vc_resize
    ...
     clear_selection
      highlight
      ...
       do_update_region
	fbcon_putcs
	 ccw_putcs_aligned
	  src = ops->fontbuffer + (scr_readw(s--) & charmask)*cellsize
	  fb_pad_aligned_buffer----[src KASAN!!!]
       update_screen
        redraw_screen
	 fbcon_switch
	  fbcon_rotate_font
	   dst = kmalloc_array(len, d_cellsize, GFP_KERNEL)
	   ops->fontbuffer = dst

[FIX]
Considering that when the rotation changes, clear_selection() should clear
the previously selected region and not consider the new rotation yet.
Therefore, the assignment to fbcon_ops for the newly set rotate can be
postponed to fbcon_rotate_font(), since the fontbuffer is regenerated
there. To avoid affecting other code paths, fbcon_set_rotate() will
temporarily continue assigning fbcon_ops based on cur_rotate not rotate.

Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
---
 drivers/video/fbdev/core/fbcon_rotate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbcon_rotate.c b/drivers/video/fbdev/core/fbcon_rotate.c
index ec3c883400f7..d76446da24d4 100644
--- a/drivers/video/fbdev/core/fbcon_rotate.c
+++ b/drivers/video/fbdev/core/fbcon_rotate.c
@@ -70,6 +70,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
 			src += s_cellsize;
 			dst += d_cellsize;
 		}
+		fbcon_rotate_ud(ops);
 		break;
 	case FB_ROTATE_CW:
 		for (i = len; i--; ) {
@@ -78,6 +79,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
 			src += s_cellsize;
 			dst += d_cellsize;
 		}
+		fbcon_rotate_cw(ops);
 		break;
 	case FB_ROTATE_CCW:
 		for (i = len; i--; ) {
@@ -86,6 +88,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
 			src += s_cellsize;
 			dst += d_cellsize;
 		}
+		fbcon_rotate_ccw(ops);
 		break;
 	}
 
@@ -97,7 +100,7 @@ void fbcon_set_rotate(struct fbcon_ops *ops)
 {
 	ops->rotate_font = fbcon_rotate_font;
 
-	switch(ops->rotate) {
+	switch (ops->cur_rotate) {
 	case FB_ROTATE_CW:
 		fbcon_rotate_cw(ops);
 		break;
-- 
2.39.2

Re: [PATCH] fbdev: Delay the setting of fbcon_ops to fix KASAN issues

[Thomas Zimmermann]

Hi

Am 05.09.25 um 04:43 schrieb Zizhi Wo:
> [BUG]
> Recently, we encountered a KASAN warning as follows:
>
> kasan_report+0xaf/0xe0 mm/kasan/report.c:588
> fb_pad_aligned_buffer+0x12f/0x150 drivers/video/fbdev/core/fbmem.c:116
> ccw_putcs_aligned drivers/video/fbdev/core/fbcon_ccw.c:119 [inline]
> ccw_putcs+0x9ac/0xbb0 drivers/video/fbdev/core/fbcon_ccw.c:175
> fbcon_putcs+0x329/0x3f0 drivers/video/fbdev/core/fbcon.c:1297
> do_update_region+0x3de/0x670 drivers/tty/vt/vt.c:623
> invert_screen+0x1de/0x600 drivers/tty/vt/vt.c:748
> highlight drivers/tty/vt/selection.c:57 [inline]
> clear_selection+0x5e/0x70 drivers/tty/vt/selection.c:81
> vc_do_resize+0xc8e/0xf40 drivers/tty/vt/vt.c:1206
> fbcon_modechanged+0x489/0x7a0 drivers/video/fbdev/core/fbcon.c:2705
> fbcon_set_all_vcs+0x1e0/0x600 drivers/video/fbdev/core/fbcon.c:2752
> fbcon_rotate_all drivers/video/fbdev/core/fbcon.c:250 [inline]
> ...
>
> reproduce[probabilistic, depending on the width and height of vc_font, as
> well as the value of "p" in do_update_region()]:

Which font sizes trigger the bug?

> 1) echo 2 > /sys/devices/virtual/graphics/fbcon/rotate_all
> 2) echo 3 > /sys/devices/virtual/graphics/fbcon/rotate_all
>
> [CAUSE]
> The root cause is that fbcon_modechanged() first sets the current rotate's
> corresponding ops. Subsequently, during vc_resize(), it may trigger
> clear_selection(), and in fbcon_putcs->ccw_putcs[rotate=3], this can result
> in an out-of-bounds access to "src". This happens because ops->fontbuffer
> is reallocated in fbcon_rotate_font():
> 1) When rotate=2, its size is (width + 7) / 8 * height
> 2) When rotate=3, its size is (height + 7) / 8 * width
>
> And the call to fbcon_rotate_font() occurs after clear_selection(). In
> other words, the fontbuffer is allocated using the size calculated from the
> previous rotation[2], but before reallocating it with the new size,
> con_putcs is already using the new rotation[3]:

We recently reworked the way rotation callbacks are set. [1] Does the 
bug still happen with [1] applied?

[1] https://patchwork.freedesktop.org/series/153056/#rev2

Best regards
Thomas

>
> rotate_all_store
>   fbcon_rotate_all
>    fbcon_set_all_vcs
>     fbcon_modechanged
>     ...
>      fbcon_set_rotate
>       fbcon_rotate_ccw
>        ops->putcs = ccw_putcs // set rotate 3 ops
>      vc_resize
>      ...
>       clear_selection
>        highlight
>        ...
>         do_update_region
> 	fbcon_putcs
> 	 ccw_putcs_aligned
> 	  src = ops->fontbuffer + (scr_readw(s--) & charmask)*cellsize
> 	  fb_pad_aligned_buffer----[src KASAN!!!]
>         update_screen
>          redraw_screen
> 	 fbcon_switch
> 	  fbcon_rotate_font
> 	   dst = kmalloc_array(len, d_cellsize, GFP_KERNEL)
> 	   ops->fontbuffer = dst
>
> [FIX]
> Considering that when the rotation changes, clear_selection() should clear
> the previously selected region and not consider the new rotation yet.
> Therefore, the assignment to fbcon_ops for the newly set rotate can be
> postponed to fbcon_rotate_font(), since the fontbuffer is regenerated
> there. To avoid affecting other code paths, fbcon_set_rotate() will
> temporarily continue assigning fbcon_ops based on cur_rotate not rotate.
>
> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
> ---
>   drivers/video/fbdev/core/fbcon_rotate.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon_rotate.c b/drivers/video/fbdev/core/fbcon_rotate.c
> index ec3c883400f7..d76446da24d4 100644
> --- a/drivers/video/fbdev/core/fbcon_rotate.c
> +++ b/drivers/video/fbdev/core/fbcon_rotate.c
> @@ -70,6 +70,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
>   			src += s_cellsize;
>   			dst += d_cellsize;
>   		}
> +		fbcon_rotate_ud(ops);
>   		break;
>   	case FB_ROTATE_CW:
>   		for (i = len; i--; ) {
> @@ -78,6 +79,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
>   			src += s_cellsize;
>   			dst += d_cellsize;
>   		}
> +		fbcon_rotate_cw(ops);
>   		break;
>   	case FB_ROTATE_CCW:
>   		for (i = len; i--; ) {
> @@ -86,6 +88,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
>   			src += s_cellsize;
>   			dst += d_cellsize;
>   		}
> +		fbcon_rotate_ccw(ops);
>   		break;
>   	}
>   
> @@ -97,7 +100,7 @@ void fbcon_set_rotate(struct fbcon_ops *ops)
>   {
>   	ops->rotate_font = fbcon_rotate_font;
>   
> -	switch(ops->rotate) {
> +	switch (ops->cur_rotate) {
>   	case FB_ROTATE_CW:
>   		fbcon_rotate_cw(ops);
>   		break;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH] fbdev: Delay the setting of fbcon_ops to fix KASAN issues

[Zizhi Wo]

在 2025/9/22 14:31, Thomas Zimmermann 写道:
> Hi
> 
> Am 05.09.25 um 04:43 schrieb Zizhi Wo:
>> [BUG]
>> Recently, we encountered a KASAN warning as follows:
>>
>> kasan_report+0xaf/0xe0 mm/kasan/report.c:588
>> fb_pad_aligned_buffer+0x12f/0x150 drivers/video/fbdev/core/fbmem.c:116
>> ccw_putcs_aligned drivers/video/fbdev/core/fbcon_ccw.c:119 [inline]
>> ccw_putcs+0x9ac/0xbb0 drivers/video/fbdev/core/fbcon_ccw.c:175
>> fbcon_putcs+0x329/0x3f0 drivers/video/fbdev/core/fbcon.c:1297
>> do_update_region+0x3de/0x670 drivers/tty/vt/vt.c:623
>> invert_screen+0x1de/0x600 drivers/tty/vt/vt.c:748
>> highlight drivers/tty/vt/selection.c:57 [inline]
>> clear_selection+0x5e/0x70 drivers/tty/vt/selection.c:81
>> vc_do_resize+0xc8e/0xf40 drivers/tty/vt/vt.c:1206
>> fbcon_modechanged+0x489/0x7a0 drivers/video/fbdev/core/fbcon.c:2705
>> fbcon_set_all_vcs+0x1e0/0x600 drivers/video/fbdev/core/fbcon.c:2752
>> fbcon_rotate_all drivers/video/fbdev/core/fbcon.c:250 [inline]
>> ...
>>
>> reproduce[probabilistic, depending on the width and height of vc_font, as
>> well as the value of "p" in do_update_region()]:
> 
> Which font sizes trigger the bug?

As far as I can remember, op.width = 32 and op.height = 12;

And I also do the TIOCL_SETSEL ioctl to set vc_sel.start && vc_sel.end

> 
>> 1) echo 2 > /sys/devices/virtual/graphics/fbcon/rotate_all
>> 2) echo 3 > /sys/devices/virtual/graphics/fbcon/rotate_all
>>
>> [CAUSE]
>> The root cause is that fbcon_modechanged() first sets the current 
>> rotate's
>> corresponding ops. Subsequently, during vc_resize(), it may trigger
>> clear_selection(), and in fbcon_putcs->ccw_putcs[rotate=3], this can 
>> result
>> in an out-of-bounds access to "src". This happens because ops->fontbuffer
>> is reallocated in fbcon_rotate_font():
>> 1) When rotate=2, its size is (width + 7) / 8 * height
>> 2) When rotate=3, its size is (height + 7) / 8 * width
>>
>> And the call to fbcon_rotate_font() occurs after clear_selection(). In
>> other words, the fontbuffer is allocated using the size calculated 
>> from the
>> previous rotation[2], but before reallocating it with the new size,
>> con_putcs is already using the new rotation[3]:
> 
> We recently reworked the way rotation callbacks are set. [1] Does the 
> bug still happen with [1] applied?
> 
> [1] https://patchwork.freedesktop.org/series/153056/#rev2

Sorry, my reproduction script has been cleaned up because some time has
passed. But the root cause of the issue is still setting ops too early,
which leads to vc_resize() calling clear_selection(), then eventually
.putcs. This uses the updated rotation-related functions on the previous
region, which may cause out-of-bounds access.

If this patch series does not ensure that the old putcs is used in the
context of clear_selection() during vc_resize(), the problem may still 
exist?

Thanks,
Zizhi Wo

> 
> Best regards
> Thomas
> 
>>
>> rotate_all_store
>>   fbcon_rotate_all
>>    fbcon_set_all_vcs
>>     fbcon_modechanged
>>     ...
>>      fbcon_set_rotate
>>       fbcon_rotate_ccw
>>        ops->putcs = ccw_putcs // set rotate 3 ops
>>      vc_resize
>>      ...
>>       clear_selection
>>        highlight
>>        ...
>>         do_update_region
>>     fbcon_putcs
>>      ccw_putcs_aligned
>>       src = ops->fontbuffer + (scr_readw(s--) & charmask)*cellsize
>>       fb_pad_aligned_buffer----[src KASAN!!!]
>>         update_screen
>>          redraw_screen
>>      fbcon_switch
>>       fbcon_rotate_font
>>        dst = kmalloc_array(len, d_cellsize, GFP_KERNEL)
>>        ops->fontbuffer = dst
>>
>> [FIX]
>> Considering that when the rotation changes, clear_selection() should 
>> clear
>> the previously selected region and not consider the new rotation yet.
>> Therefore, the assignment to fbcon_ops for the newly set rotate can be
>> postponed to fbcon_rotate_font(), since the fontbuffer is regenerated
>> there. To avoid affecting other code paths, fbcon_set_rotate() will
>> temporarily continue assigning fbcon_ops based on cur_rotate not rotate.
>>
>> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
>> ---
>>   drivers/video/fbdev/core/fbcon_rotate.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/core/fbcon_rotate.c 
>> b/drivers/video/fbdev/core/fbcon_rotate.c
>> index ec3c883400f7..d76446da24d4 100644
>> --- a/drivers/video/fbdev/core/fbcon_rotate.c
>> +++ b/drivers/video/fbdev/core/fbcon_rotate.c
>> @@ -70,6 +70,7 @@ static int fbcon_rotate_font(struct fb_info *info, 
>> struct vc_data *vc)
>>               src += s_cellsize;
>>               dst += d_cellsize;
>>           }
>> +        fbcon_rotate_ud(ops);
>>           break;
>>       case FB_ROTATE_CW:
>>           for (i = len; i--; ) {
>> @@ -78,6 +79,7 @@ static int fbcon_rotate_font(struct fb_info *info, 
>> struct vc_data *vc)
>>               src += s_cellsize;
>>               dst += d_cellsize;
>>           }
>> +        fbcon_rotate_cw(ops);
>>           break;
>>       case FB_ROTATE_CCW:
>>           for (i = len; i--; ) {
>> @@ -86,6 +88,7 @@ static int fbcon_rotate_font(struct fb_info *info, 
>> struct vc_data *vc)
>>               src += s_cellsize;
>>               dst += d_cellsize;
>>           }
>> +        fbcon_rotate_ccw(ops);
>>           break;
>>       }
>> @@ -97,7 +100,7 @@ void fbcon_set_rotate(struct fbcon_ops *ops)
>>   {
>>       ops->rotate_font = fbcon_rotate_font;
>> -    switch(ops->rotate) {
>> +    switch (ops->cur_rotate) {
>>       case FB_ROTATE_CW:
>>           fbcon_rotate_cw(ops);
>>           break;
>