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

syzbot has found a NULL pointer dereference bug in fbcon [1].

This issue is caused by ops->putcs being a NULL pointer.
We need to check the pointer before using it.

[1] https://syzkaller.appspot.com/bug?extid=3d613ae53c031502687a

Cc: stable@vger.kernel.org
Reported-and-tested-by: syzbot+3d613ae53c031502687a@syzkaller.appspotmail.com
Signed-off-by: Qianqiang Liu <redacted>
---
 drivers/video/fbdev/core/fbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 3f7333dca508..96c1262cc981 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c

@@ -1284,7 +1284,7 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (!fbcon_is_inactive(vc, info))
+	if (!fbcon_is_inactive(vc, info) && ops->putcs)
 		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
 			   get_color(vc, info, scr_readw(s), 1),
 			   get_color(vc, info, scr_readw(s), 0));

-- 
2.39.2

On 9/16/24 03:10, Qianqiang Liu wrote:

I think this patch just hides the real problem.
How could putcs have become NULL ?

Helge

  		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
  			   get_color(vc, info, scr_readw(s), 1),
  			   get_color(vc, info, scr_readw(s), 0));

Hi, 

I simplified the C reproducer as follows:

#include <stdint.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <linux/fb.h>
#include <linux/tiocl.h>
#include <sys/ioctl.h>

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

int main()
{
        write(1, "executing program\n", sizeof("executing program\n") - 1);

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

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

        int fd1 = open("/dev/tty1", O_RDWR, 0);

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

        ioctl(fd1, TIOCLINUX, &param);

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

        return 0;
}

But I still need time to debug the kernel code..

-- 
Best,
Qianqiang Liu

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 <redacted>
---
 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

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 <redacted>
---
  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

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 <redacted>
---
 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

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 <redacted>
---
  Changes since v2:
  - Document the commit message in more detail

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

Thanks!
Helge