[PATCH] fbcon: Set fb_display[i]->mode to NULL when the mode is released

Quanmin Yan posted 1 patch 4 months, 2 weeks ago
There is a newer version of this series
drivers/video/fbdev/core/fbcon.c  | 11 +++++++++++
drivers/video/fbdev/core/modedb.c |  7 +++++++
include/linux/fbcon.h             |  2 ++
3 files changed, 20 insertions(+)
[PATCH] fbcon: Set fb_display[i]->mode to NULL when the mode is released
Posted by Quanmin Yan 4 months, 2 weeks ago
Recently, we discovered the following issue through syzkaller:

BUG: KASAN: slab-use-after-free in fb_mode_is_equal+0x285/0x2f0
Read of size 4 at addr ff11000001b3c69c by task syz.xxx
...
Call Trace:
 <TASK>
 dump_stack_lvl+0xab/0xe0
 print_address_description.constprop.0+0x2c/0x390
 print_report+0xb9/0x280
 kasan_report+0xb8/0xf0
 fb_mode_is_equal+0x285/0x2f0
 fbcon_mode_deleted+0x129/0x180
 fb_set_var+0xe7f/0x11d0
 do_fb_ioctl+0x6a0/0x750
 fb_ioctl+0xe0/0x140
 __x64_sys_ioctl+0x193/0x210
 do_syscall_64+0x5f/0x9c0
 entry_SYSCALL_64_after_hwframe+0x76/0x7e

The issue occurs in the function fb_mode_is_equal(p->mode, mode), I also
noticed that when freeing the memory related to fb_info->modelist, there's
no attempt to set the corresponding fb_display[i]->mode to NULL after
freeing. Based on analysis, the root cause of this bug appears to be that
a certain p->mode has become a wild pointer.

I've identified two code paths for freeing modelist->mode:
1. fb_delete_videomode - removes videomode entry from modelist.
2. fb_destroy_modelist - destroys the entire modelist.

Analysis shows that fb_delete_videomode path should have been fixed in
a previous patch[1]. Therefore, the current bug is likely triggered
through the fb_destroy_modelist path. I've found a reproducible test case:
1. With /dev/fb0 already registered in the system, load a kernel module
   to register a new device /dev/fb1;
2. Set fb1's mode to the global fb_display[] array (via FBIOPUT_CON2FBMAP);
3. Switch console from fb to VGA (to allow normal rmmod of the ko);
4. Unload the kernel module - at this point fb1's modelist is freed, leaving
   a wild pointer in fb_display[];
5. Trigger the bug via system calls through fb0 attempting to delete a mode
   from fb0.

To prevent similar issues from recurring, consider traversing fb_display[]
whenever releasing a mode from fb_info. If the corresponding mode exists
in fb_display[], set its pointer to NULL.

[1] https://lore.kernel.org/all/20210712085544.2828-1-thunder.leizhen@huawei.com/

Signed-off-by: Quanmin Yan <yanquanmin1@huawei.com>
---
This is my first time working on fb issues. If there are any misunderstandings
in my analysis, I would appreciate corrections from the community.

 drivers/video/fbdev/core/fbcon.c  | 11 +++++++++++
 drivers/video/fbdev/core/modedb.c |  7 +++++++
 include/linux/fbcon.h             |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index b062b05f4128..bfbf79d6cd05 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2803,6 +2803,17 @@ int fbcon_mode_deleted(struct fb_info *info,
 	return found;
 }
 
+void fb_display_clean_videomode(struct fb_videomode *m)
+{
+	struct fbcon_display *p;
+
+	for (int i = first_fb_vc; i <= last_fb_vc; i++) {
+		p = &fb_display[i];
+		if (p->mode == m)
+			p->mode = NULL;
+	}
+}
+
 #ifdef CONFIG_VT_HW_CONSOLE_BINDING
 static void fbcon_unbind(void)
 {
diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
index 53a610948c4a..5a0ee96ebefa 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/fb.h>
 #include <linux/kernel.h>
+#include <linux/fbcon.h>
 
 #undef DEBUG
 
@@ -1100,6 +1101,7 @@ void fb_delete_videomode(const struct fb_videomode *mode,
 		modelist = list_entry(pos, struct fb_modelist, list);
 		m = &modelist->mode;
 		if (fb_mode_is_equal(m, mode)) {
+			fb_display_clean_videomode(m);
 			list_del(pos);
 			kfree(pos);
 		}
@@ -1113,8 +1115,13 @@ void fb_delete_videomode(const struct fb_videomode *mode,
 void fb_destroy_modelist(struct list_head *head)
 {
 	struct list_head *pos, *n;
+	struct fb_modelist *modelist;
+	struct fb_videomode *m;
 
 	list_for_each_safe(pos, n, head) {
+		modelist = list_entry(pos, struct fb_modelist, list);
+		m = &modelist->mode;
+		fb_display_clean_videomode(m);
 		list_del(pos);
 		kfree(pos);
 	}
diff --git a/include/linux/fbcon.h b/include/linux/fbcon.h
index 81f0e698acbf..2b5e93aeaaff 100644
--- a/include/linux/fbcon.h
+++ b/include/linux/fbcon.h
@@ -18,6 +18,7 @@ void fbcon_suspended(struct fb_info *info);
 void fbcon_resumed(struct fb_info *info);
 int fbcon_mode_deleted(struct fb_info *info,
 		       struct fb_videomode *mode);
+void fb_display_clean_videomode(struct fb_videomode *m);
 void fbcon_new_modelist(struct fb_info *info);
 void fbcon_get_requirement(struct fb_info *info,
 			   struct fb_blit_caps *caps);
@@ -38,6 +39,7 @@ static inline void fbcon_suspended(struct fb_info *info) {}
 static inline void fbcon_resumed(struct fb_info *info) {}
 static inline int fbcon_mode_deleted(struct fb_info *info,
 				     struct fb_videomode *mode) { return 0; }
+static inline void fb_display_clean_videomode(struct fb_videomode *m) {}
 static inline void fbcon_new_modelist(struct fb_info *info) {}
 static inline void fbcon_get_requirement(struct fb_info *info,
 					 struct fb_blit_caps *caps) {}
-- 
2.43.0
Re: [PATCH] fbcon: Set fb_display[i]->mode to NULL when the mode is released
Posted by Thomas Zimmermann 4 months, 2 weeks ago
Hi,

thanks for the report.

Am 23.09.25 um 13:06 schrieb Quanmin Yan:
> Recently, we discovered the following issue through syzkaller:
>
> BUG: KASAN: slab-use-after-free in fb_mode_is_equal+0x285/0x2f0
> Read of size 4 at addr ff11000001b3c69c by task syz.xxx
> ...
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0xab/0xe0
>   print_address_description.constprop.0+0x2c/0x390
>   print_report+0xb9/0x280
>   kasan_report+0xb8/0xf0
>   fb_mode_is_equal+0x285/0x2f0
>   fbcon_mode_deleted+0x129/0x180
>   fb_set_var+0xe7f/0x11d0
>   do_fb_ioctl+0x6a0/0x750
>   fb_ioctl+0xe0/0x140
>   __x64_sys_ioctl+0x193/0x210
>   do_syscall_64+0x5f/0x9c0
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> The issue occurs in the function fb_mode_is_equal(p->mode, mode), I also
> noticed that when freeing the memory related to fb_info->modelist, there's
> no attempt to set the corresponding fb_display[i]->mode to NULL after
> freeing. Based on analysis, the root cause of this bug appears to be that
> a certain p->mode has become a wild pointer.
>
> I've identified two code paths for freeing modelist->mode:
> 1. fb_delete_videomode - removes videomode entry from modelist.
> 2. fb_destroy_modelist - destroys the entire modelist.

What about fb_new_modelist()? [1] It's called from store_modes() and the 
whole logic in the caller seems fragile to me. This could leave p->mode 
set when it should be cleared; and vice versa.

[1] 
https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbmem.c#L712
[2] 
https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbsysfs.c#L113

>
> Analysis shows that fb_delete_videomode path should have been fixed in
> a previous patch[1]. Therefore, the current bug is likely triggered
> through the fb_destroy_modelist path. I've found a reproducible test case:
> 1. With /dev/fb0 already registered in the system, load a kernel module
>     to register a new device /dev/fb1;
> 2. Set fb1's mode to the global fb_display[] array (via FBIOPUT_CON2FBMAP);
> 3. Switch console from fb to VGA (to allow normal rmmod of the ko);
> 4. Unload the kernel module - at this point fb1's modelist is freed, leaving
>     a wild pointer in fb_display[];
> 5. Trigger the bug via system calls through fb0 attempting to delete a mode
>     from fb0.
>
> To prevent similar issues from recurring, consider traversing fb_display[]
> whenever releasing a mode from fb_info. If the corresponding mode exists
> in fb_display[], set its pointer to NULL.
>
> [1] https://lore.kernel.org/all/20210712085544.2828-1-thunder.leizhen@huawei.com/
>
> Signed-off-by: Quanmin Yan <yanquanmin1@huawei.com>
> ---
> This is my first time working on fb issues. If there are any misunderstandings
> in my analysis, I would appreciate corrections from the community.
>
>   drivers/video/fbdev/core/fbcon.c  | 11 +++++++++++
>   drivers/video/fbdev/core/modedb.c |  7 +++++++
>   include/linux/fbcon.h             |  2 ++
>   3 files changed, 20 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index b062b05f4128..bfbf79d6cd05 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2803,6 +2803,17 @@ int fbcon_mode_deleted(struct fb_info *info,
>   	return found;
>   }
>   
> +void fb_display_clean_videomode(struct fb_videomode *m)

Rather fbcon_delete_mode

> +{
> +	struct fbcon_display *p;
> +
> +	for (int i = first_fb_vc; i <= last_fb_vc; i++) {

I think this code also needs to test the fb_info, or it might clear 
another display's mode. See [3] for an example

[3] 
https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbcon.c#L2786

> +		p = &fb_display[i];
> +		if (p->mode == m)
> +			p->mode = NULL;
> +	}
> +}
> +
>   #ifdef CONFIG_VT_HW_CONSOLE_BINDING
>   static void fbcon_unbind(void)
>   {
> diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
> index 53a610948c4a..5a0ee96ebefa 100644
> --- a/drivers/video/fbdev/core/modedb.c
> +++ b/drivers/video/fbdev/core/modedb.c
> @@ -16,6 +16,7 @@
>   #include <linux/slab.h>
>   #include <linux/fb.h>
>   #include <linux/kernel.h>
> +#include <linux/fbcon.h>
>   
>   #undef DEBUG
>   
> @@ -1100,6 +1101,7 @@ void fb_delete_videomode(const struct fb_videomode *mode,
>   		modelist = list_entry(pos, struct fb_modelist, list);
>   		m = &modelist->mode;
>   		if (fb_mode_is_equal(m, mode)) {
> +			fb_display_clean_videomode(m);

There's only one caller of fb_delete_videomode(). I think this call 
should be right before fb_delete_videomode().

>   			list_del(pos);
>   			kfree(pos);
>   		}
> @@ -1113,8 +1115,13 @@ void fb_delete_videomode(const struct fb_videomode *mode,
>   void fb_destroy_modelist(struct list_head *head)
>   {
>   	struct list_head *pos, *n;
> +	struct fb_modelist *modelist;
> +	struct fb_videomode *m;
>   
>   	list_for_each_safe(pos, n, head) {
> +		modelist = list_entry(pos, struct fb_modelist, list);
> +		m = &modelist->mode;
> +		fb_display_clean_videomode(m);

Same here: I think fb_destroy_modelist() should only release the mode. 
Clearing the fbcon mode should be done by the caller.

Best regards
Thomas

>   		list_del(pos);
>   		kfree(pos);
>   	}
> diff --git a/include/linux/fbcon.h b/include/linux/fbcon.h
> index 81f0e698acbf..2b5e93aeaaff 100644
> --- a/include/linux/fbcon.h
> +++ b/include/linux/fbcon.h
> @@ -18,6 +18,7 @@ void fbcon_suspended(struct fb_info *info);
>   void fbcon_resumed(struct fb_info *info);
>   int fbcon_mode_deleted(struct fb_info *info,
>   		       struct fb_videomode *mode);
> +void fb_display_clean_videomode(struct fb_videomode *m);
>   void fbcon_new_modelist(struct fb_info *info);
>   void fbcon_get_requirement(struct fb_info *info,
>   			   struct fb_blit_caps *caps);
> @@ -38,6 +39,7 @@ static inline void fbcon_suspended(struct fb_info *info) {}
>   static inline void fbcon_resumed(struct fb_info *info) {}
>   static inline int fbcon_mode_deleted(struct fb_info *info,
>   				     struct fb_videomode *mode) { return 0; }
> +static inline void fb_display_clean_videomode(struct fb_videomode *m) {}
>   static inline void fbcon_new_modelist(struct fb_info *info) {}
>   static inline void fbcon_get_requirement(struct fb_info *info,
>   					 struct fb_blit_caps *caps) {}

-- 
--
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] fbcon: Set fb_display[i]->mode to NULL when the mode is released
Posted by Quanmin Yan 4 months ago
Hi Thomas,

在 2025/9/24 17:17, Thomas Zimmermann 写道:
> Hi,
>
> thanks for the report.
>
> Am 23.09.25 um 13:06 schrieb Quanmin Yan:
>> Recently, we discovered the following issue through syzkaller:
>>
>> BUG: KASAN: slab-use-after-free in fb_mode_is_equal+0x285/0x2f0
>> Read of size 4 at addr ff11000001b3c69c by task syz.xxx
>> ...
>> Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0xab/0xe0
>>   print_address_description.constprop.0+0x2c/0x390
>>   print_report+0xb9/0x280
>>   kasan_report+0xb8/0xf0
>>   fb_mode_is_equal+0x285/0x2f0
>>   fbcon_mode_deleted+0x129/0x180
>>   fb_set_var+0xe7f/0x11d0
>>   do_fb_ioctl+0x6a0/0x750
>>   fb_ioctl+0xe0/0x140
>>   __x64_sys_ioctl+0x193/0x210
>>   do_syscall_64+0x5f/0x9c0
>>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> The issue occurs in the function fb_mode_is_equal(p->mode, mode), I also
>> noticed that when freeing the memory related to fb_info->modelist, 
>> there's
>> no attempt to set the corresponding fb_display[i]->mode to NULL after
>> freeing. Based on analysis, the root cause of this bug appears to be 
>> that
>> a certain p->mode has become a wild pointer.
>>
>> I've identified two code paths for freeing modelist->mode:
>> 1. fb_delete_videomode - removes videomode entry from modelist.
>> 2. fb_destroy_modelist - destroys the entire modelist.
>
> What about fb_new_modelist()? [1] It's called from store_modes() and 
> the whole logic in the caller seems fragile to me. This could leave 
> p->mode set when it should be cleared; and vice versa.
>
> [1] 
> https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbmem.c#L712
> [2] 
> https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbsysfs.c#L113 
>
>
After testing, this path does not exhibit the issue because in 
fbcon_new_modelist [1], the
modes in fb_display[] get refreshed and won't leave wild pointers. Even 
if fb_new_modelist()
fails, the original modelist will be restored [2].

[1] 
https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbmem.c#L736
[2] 
https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbsysfs.c#L115 

>>
>> Analysis shows that fb_delete_videomode path should have been fixed in
>> a previous patch[1]. Therefore, the current bug is likely triggered
>> through the fb_destroy_modelist path. I've found a reproducible test 
>> case:
>> 1. With /dev/fb0 already registered in the system, load a kernel module
>>     to register a new device /dev/fb1;
>> 2. Set fb1's mode to the global fb_display[] array (via 
>> FBIOPUT_CON2FBMAP);
>> 3. Switch console from fb to VGA (to allow normal rmmod of the ko);
>> 4. Unload the kernel module - at this point fb1's modelist is freed, 
>> leaving
>>     a wild pointer in fb_display[];
>> 5. Trigger the bug via system calls through fb0 attempting to delete 
>> a mode
>>     from fb0.
>>
>> To prevent similar issues from recurring, consider traversing 
>> fb_display[]
>> whenever releasing a mode from fb_info. If the corresponding mode exists
>> in fb_display[], set its pointer to NULL.
>>
>> [1] 
>> https://lore.kernel.org/all/20210712085544.2828-1-thunder.leizhen@huawei.com/
>>
>> Signed-off-by: Quanmin Yan <yanquanmin1@huawei.com>
>> ---
>> This is my first time working on fb issues. If there are any 
>> misunderstandings
>> in my analysis, I would appreciate corrections from the community.
>>
>>   drivers/video/fbdev/core/fbcon.c  | 11 +++++++++++
>>   drivers/video/fbdev/core/modedb.c |  7 +++++++
>>   include/linux/fbcon.h             |  2 ++
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbcon.c 
>> b/drivers/video/fbdev/core/fbcon.c
>> index b062b05f4128..bfbf79d6cd05 100644
>> --- a/drivers/video/fbdev/core/fbcon.c
>> +++ b/drivers/video/fbdev/core/fbcon.c
>> @@ -2803,6 +2803,17 @@ int fbcon_mode_deleted(struct fb_info *info,
>>       return found;
>>   }
>>   +void fb_display_clean_videomode(struct fb_videomode *m)
>
> Rather fbcon_delete_mode
>
>> +{
>> +    struct fbcon_display *p;
>> +
>> +    for (int i = first_fb_vc; i <= last_fb_vc; i++) {
>
> I think this code also needs to test the fb_info, or it might clear 
> another display's mode. See [3] for an example
>
> [3] 
> https://elixir.bootlin.com/linux/v6.16.8/source/drivers/video/fbdev/core/fbcon.c#L2786
>
>> +        p = &fb_display[i];
>> +        if (p->mode == m)
>> +            p->mode = NULL;
>> +    }
>> +}
>> +
>>   #ifdef CONFIG_VT_HW_CONSOLE_BINDING
>>   static void fbcon_unbind(void)
>>   {
>> diff --git a/drivers/video/fbdev/core/modedb.c 
>> b/drivers/video/fbdev/core/modedb.c
>> index 53a610948c4a..5a0ee96ebefa 100644
>> --- a/drivers/video/fbdev/core/modedb.c
>> +++ b/drivers/video/fbdev/core/modedb.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/fb.h>
>>   #include <linux/kernel.h>
>> +#include <linux/fbcon.h>
>>     #undef DEBUG
>>   @@ -1100,6 +1101,7 @@ void fb_delete_videomode(const struct 
>> fb_videomode *mode,
>>           modelist = list_entry(pos, struct fb_modelist, list);
>>           m = &modelist->mode;
>>           if (fb_mode_is_equal(m, mode)) {
>> +            fb_display_clean_videomode(m);
>
> There's only one caller of fb_delete_videomode(). I think this call 
> should be right before fb_delete_videomode().
This path has already been fixed [3], so this modification is no longer 
necessary and will be
removed in the next version of the patch.

[3] 
https://lore.kernel.org/all/20210712085544.2828-1-thunder.leizhen@huawei.com/

Additionally, in the next version of the patch, I have incorporated your 
other suggestions
and made corresponding adjustments. I will send the updated patch set 
shortly.

Best regards,
Quanmin Yan
>
>>               list_del(pos);
>>               kfree(pos);
>>           }
>> @@ -1113,8 +1115,13 @@ void fb_delete_videomode(const struct 
>> fb_videomode *mode,
>>   void fb_destroy_modelist(struct list_head *head)
>>   {
>>       struct list_head *pos, *n;
>> +    struct fb_modelist *modelist;
>> +    struct fb_videomode *m;
>>         list_for_each_safe(pos, n, head) {
>> +        modelist = list_entry(pos, struct fb_modelist, list);
>> +        m = &modelist->mode;
>> +        fb_display_clean_videomode(m);
>
> Same here: I think fb_destroy_modelist() should only release the mode. 
> Clearing the fbcon mode should be done by the caller.
>
> Best regards
> Thomas
>
>>           list_del(pos);
>>           kfree(pos);
>>       }
>> diff --git a/include/linux/fbcon.h b/include/linux/fbcon.h
>> index 81f0e698acbf..2b5e93aeaaff 100644
>> --- a/include/linux/fbcon.h
>> +++ b/include/linux/fbcon.h
>> @@ -18,6 +18,7 @@ void fbcon_suspended(struct fb_info *info);
>>   void fbcon_resumed(struct fb_info *info);
>>   int fbcon_mode_deleted(struct fb_info *info,
>>                  struct fb_videomode *mode);
>> +void fb_display_clean_videomode(struct fb_videomode *m);
>>   void fbcon_new_modelist(struct fb_info *info);
>>   void fbcon_get_requirement(struct fb_info *info,
>>                  struct fb_blit_caps *caps);
>> @@ -38,6 +39,7 @@ static inline void fbcon_suspended(struct fb_info 
>> *info) {}
>>   static inline void fbcon_resumed(struct fb_info *info) {}
>>   static inline int fbcon_mode_deleted(struct fb_info *info,
>>                        struct fb_videomode *mode) { return 0; }
>> +static inline void fb_display_clean_videomode(struct fb_videomode 
>> *m) {}
>>   static inline void fbcon_new_modelist(struct fb_info *info) {}
>>   static inline void fbcon_get_requirement(struct fb_info *info,
>>                        struct fb_blit_caps *caps) {}
>