In the pxafb_probe function, it calls the pxafb_init_fbinfo function,
after which &fbi->task is associated with pxafb_task. Moreover,
within this pxafb_init_fbinfo function, the pxafb_blank function
within the &pxafb_ops struct is capable of scheduling work.

If we remove the module which will call pxafb_remove to make cleanup,
it will call unregister_framebuffer function which can call
do_unregister_framebuffer to free fbi->fb through
put_fb_info(fb_info), while the work mentioned above will be used.
The sequence of operations that may lead to a UAF bug is as follows:

CPU0                                                CPU1

                                   | pxafb_task
pxafb_remove                       |
unregister_framebuffer(info)       |
do_unregister_framebuffer(fb_info) |
put_fb_info(fb_info)               |
// free fbi->fb                    | set_ctrlr_state(fbi, state)
                                   | __pxafb_lcd_power(fbi, 0)
                                   | fbi->lcd_power(on, &fbi->fb.var)
                                   | //use fbi->fb

Fix it by ensuring that the work is canceled before proceeding
with the cleanup in pxafb_remove.

Note that only root user can remove the driver at runtime.

Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
---
 drivers/video/fbdev/pxafb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 2ef56fa28aff..5ce02495cda6 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2403,6 +2403,7 @@ static void pxafb_remove(struct platform_device *dev)
 	info = &fbi->fb;
 
 	pxafb_overlay_exit(fbi);
+	cancel_work_sync(&fbi->task);
 	unregister_framebuffer(info);
 
 	pxafb_disable_controller(fbi);
-- 
2.25.1

On 9/11/24 16:29, Kaixin Wang wrote:
> In the pxafb_probe function, it calls the pxafb_init_fbinfo function,
> after which &fbi->task is associated with pxafb_task. Moreover,
> within this pxafb_init_fbinfo function, the pxafb_blank function
> within the &pxafb_ops struct is capable of scheduling work.
>
> If we remove the module which will call pxafb_remove to make cleanup,
> it will call unregister_framebuffer function which can call
> do_unregister_framebuffer to free fbi->fb through
> put_fb_info(fb_info), while the work mentioned above will be used.
> The sequence of operations that may lead to a UAF bug is as follows:
>
> CPU0                                                CPU1
>
>                                     | pxafb_task
> pxafb_remove                       |
> unregister_framebuffer(info)       |
> do_unregister_framebuffer(fb_info) |
> put_fb_info(fb_info)               |
> // free fbi->fb                    | set_ctrlr_state(fbi, state)
>                                     | __pxafb_lcd_power(fbi, 0)
>                                     | fbi->lcd_power(on, &fbi->fb.var)
>                                     | //use fbi->fb
>
> Fix it by ensuring that the work is canceled before proceeding
> with the cleanup in pxafb_remove.
>
> Note that only root user can remove the driver at runtime.
>
> Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
> ---
>   drivers/video/fbdev/pxafb.c | 1 +
>   1 file changed, 1 insertion(+)

I've added the patch to the fbdev git tree, but changed the title to:
"fbdev: pxafb: Fix possible use after free in pxafb_task()"

Thanks!
Helge

At 2024-09-12 01:38:26, "Helge Deller" <deller@gmx.de> wrote:
>On 9/11/24 16:29, Kaixin Wang wrote:
>> In the pxafb_probe function, it calls the pxafb_init_fbinfo function,
>> after which &fbi->task is associated with pxafb_task. Moreover,
>> within this pxafb_init_fbinfo function, the pxafb_blank function
>> within the &pxafb_ops struct is capable of scheduling work.
>>
>> If we remove the module which will call pxafb_remove to make cleanup,
>> it will call unregister_framebuffer function which can call
>> do_unregister_framebuffer to free fbi->fb through
>> put_fb_info(fb_info), while the work mentioned above will be used.
>> The sequence of operations that may lead to a UAF bug is as follows:
>>
>> CPU0                                                CPU1
>>
>>                                     | pxafb_task
>> pxafb_remove                       |
>> unregister_framebuffer(info)       |
>> do_unregister_framebuffer(fb_info) |
>> put_fb_info(fb_info)               |
>> // free fbi->fb                    | set_ctrlr_state(fbi, state)
>>                                     | __pxafb_lcd_power(fbi, 0)
>>                                     | fbi->lcd_power(on, &fbi->fb.var)
>>                                     | //use fbi->fb
>>
>> Fix it by ensuring that the work is canceled before proceeding
>> with the cleanup in pxafb_remove.
>>
>> Note that only root user can remove the driver at runtime.
>>
>> Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
>> ---
>>   drivers/video/fbdev/pxafb.c | 1 +
>>   1 file changed, 1 insertion(+)
>
>I've added the patch to the fbdev git tree, but changed the title to:
>"fbdev: pxafb: Fix possible use after free in pxafb_task()"
>
>Thanks!
>Helge
>
>


Thanks for the review! I appreciate the adjustment to the patch title to make it more precise.

Best regards,
Kaixin Wang