1. Patchew
  2. linux
  3. View series

[PATCH v2] fbcon: Fix a NULL pointer dereference issue in fbcon_putcs

Qianqiang Liu posted 1 patch 2 months ago
There is a newer version of this series
drivers/video/fbdev/core/fbcon.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH v2] fbcon: Fix a NULL pointer dereference issue in fbcon_putcs
Posted by Qianqiang Liu 2 months ago 
syzbot has found a NULL pointer dereference bug in fbcon.

This issue is caused by ops->putcs being a NULL pointer.
We need to ensure it is initialized properly.

Reported-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3d613ae53c031502687a
Tested-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
---
 Changes since v1:
 - Initialize ops->putcs by calling set_blitting_type()
---
 drivers/video/fbdev/core/fbcon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 2e093535884b..d9abae2516d8 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -861,6 +861,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
 			return err;
 
 		fbcon_add_cursor_work(info);
+	} else if (vc) {
+		set_blitting_type(vc, info);
 	}
 
 	con2fb_map[unit] = newidx;
-- 
2.34.1
Re: [PATCH v2] fbcon: Fix a NULL pointer dereference issue in fbcon_putcs
Posted by Helge Deller 2 months ago 
Hi Qianqiang,

On 9/24/24 18:13, Qianqiang Liu wrote:
> syzbot has found a NULL pointer dereference bug in fbcon.
>
> This issue is caused by ops->putcs being a NULL pointer.
> We need to ensure it is initialized properly.
>
> Reported-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=3d613ae53c031502687a
> Tested-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
>   Changes since v1:
>   - Initialize ops->putcs by calling set_blitting_type()

Thanks a lot tracking this issue down!

At first sight your patch seems correct.
But could you please document in the patch description what exactly (and why)
something goes wrong and how your patch fixes it?
E.g. why was opt->putcs missed to be initialized even earlier and why does
it need initialization now?

You did a good work in producing a reduced testcase.
If it's quite small, it's a good idea to even include it in the
commit message?

Helge

> ---
>   drivers/video/fbdev/core/fbcon.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 2e093535884b..d9abae2516d8 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -861,6 +861,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
>   			return err;
>
>   		fbcon_add_cursor_work(info);
> +	} else if (vc) {
> +		set_blitting_type(vc, info);
>   	}
>
>   	con2fb_map[unit] = newidx;
[PATCH v3] fbcon: Fix a NULL pointer dereference issue in fbcon_putcs
Posted by Qianqiang Liu 2 months ago 
syzbot has found a NULL pointer dereference bug in fbcon.
Here is the simplified C reproducer:

struct param {
	uint8_t type;
	struct tiocl_selection ts;
};

int main()
{
	struct fb_con2fbmap con2fb;
	struct param param;

	int fd = open("/dev/fb1", 0, 0);

	con2fb.console = 0x19;
	con2fb.framebuffer = 0;
	ioctl(fd, FBIOPUT_CON2FBMAP, &con2fb);

	param.type = 2;
	param.ts.xs = 0; param.ts.ys = 0;
	param.ts.xe = 0; param.ts.ye = 0;
	param.ts.sel_mode = 0;

	int fd1 = open("/dev/tty1", O_RDWR, 0);
	ioctl(fd1, TIOCLINUX, &param);

	con2fb.console = 1;
	con2fb.framebuffer = 0;
	ioctl(fd, FBIOPUT_CON2FBMAP, &con2fb);

	return 0;
}

After calling ioctl(fd1, TIOCLINUX, &param), the subsequent ioctl(fd, FBIOPUT_CON2FBMAP, &con2fb)
causes the kernel to follow a different execution path:

 set_con2fb_map
  -> con2fb_init_display
   -> fbcon_set_disp
    -> redraw_screen
     -> hide_cursor
      -> clear_selection
       -> highlight
        -> invert_screen
         -> do_update_region
          -> fbcon_putcs
           -> ops->putcs

Since ops->putcs is a NULL pointer, this leads to a kernel panic.
To prevent this, we need to call set_blitting_type() within set_con2fb_map()
to properly initialize ops->putcs.

Reported-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3d613ae53c031502687a
Tested-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
---
 Changes since v2:
 - Document the commit message in more detail
---
 Changes since v1:
 - Initialize ops->putcs by calling set_blitting_type()
---
 drivers/video/fbdev/core/fbcon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 2e093535884b..d9abae2516d8 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -861,6 +861,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
 			return err;
 
 		fbcon_add_cursor_work(info);
+	} else if (vc) {
+		set_blitting_type(vc, info);
 	}
 
 	con2fb_map[unit] = newidx;
-- 
2.34.1
Re: [PATCH v3] fbcon: Fix a NULL pointer dereference issue in fbcon_putcs
Posted by Helge Deller 2 months ago 
On 9/25/24 07:29, Qianqiang Liu wrote:
> syzbot has found a NULL pointer dereference bug in fbcon.
> Here is the simplified C reproducer:
>
> struct param {
> 	uint8_t type;
> 	struct tiocl_selection ts;
> };
>
> int main()
> {
> 	struct fb_con2fbmap con2fb;
> 	struct param param;
>
> 	int fd = open("/dev/fb1", 0, 0);
>
> 	con2fb.console = 0x19;
> 	con2fb.framebuffer = 0;
> 	ioctl(fd, FBIOPUT_CON2FBMAP, &con2fb);
>
> 	param.type = 2;
> 	param.ts.xs = 0; param.ts.ys = 0;
> 	param.ts.xe = 0; param.ts.ye = 0;
> 	param.ts.sel_mode = 0;
>
> 	int fd1 = open("/dev/tty1", O_RDWR, 0);
> 	ioctl(fd1, TIOCLINUX, &param);
>
> 	con2fb.console = 1;
> 	con2fb.framebuffer = 0;
> 	ioctl(fd, FBIOPUT_CON2FBMAP, &con2fb);
>
> 	return 0;
> }
>
> After calling ioctl(fd1, TIOCLINUX, &param), the subsequent ioctl(fd, FBIOPUT_CON2FBMAP, &con2fb)
> causes the kernel to follow a different execution path:
>
>   set_con2fb_map
>    -> con2fb_init_display
>     -> fbcon_set_disp
>      -> redraw_screen
>       -> hide_cursor
>        -> clear_selection
>         -> highlight
>          -> invert_screen
>           -> do_update_region
>            -> fbcon_putcs
>             -> ops->putcs
>
> Since ops->putcs is a NULL pointer, this leads to a kernel panic.
> To prevent this, we need to call set_blitting_type() within set_con2fb_map()
> to properly initialize ops->putcs.
>
> Reported-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=3d613ae53c031502687a
> Tested-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
>   Changes since v2:
>   - Document the commit message in more detail

Queued up in for-next branch of fbdev git tree.

Thanks!
Helge


> ---
>   Changes since v1:
>   - Initialize ops->putcs by calling set_blitting_type()
> ---
>   drivers/video/fbdev/core/fbcon.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 2e093535884b..d9abae2516d8 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -861,6 +861,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
>   			return err;
>
>   		fbcon_add_cursor_work(info);
> +	} else if (vc) {
> +		set_blitting_type(vc, info);
>   	}
>
>   	con2fb_map[unit] = newidx;

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.