[PATCH] fbdev:modedb: fix a possible UAF in fb_find_mode()

[PATCH] fbdev:modedb: fix a possible UAF in fb_find_mode()

[Tuo Li]

If mode_option is NULL, it is assigned from mode_option_buf:

  if (!mode_option) {
    fb_get_options(NULL, &mode_option_buf);
    mode_option = mode_option_buf;
  }

Later, name is assigned from mode_option:

  const char *name = mode_option;

However, mode_option_buf is freed before name is no longer used:

  kfree(mode_option_buf);

while name is still accessed by:

  if ((name_matches(db[i], name, namelen) ||

Since name aliases mode_option_buf, this may result in a
use-after-free.

Fix this by moving the kfree(mode_option_buf) call behind the access, and
add corresponding cleanup before early returns.

Signed-off-by: Tuo Li <islituo@gmail.com>
---
 drivers/video/fbdev/core/modedb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
index 703d0b7aec32..91f6c78f9e19 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -724,7 +724,6 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 			res_specified = 1;
 		}
 done:
-		kfree(mode_option_buf);
 		if (cvt) {
 			struct fb_videomode cvt_mode;
 			int ret;
@@ -749,6 +748,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 
 			if (!ret && !fb_try_mode(var, info, &cvt_mode, bpp)) {
 				DPRINTK("modedb CVT: CVT mode ok\n");
+				kfree(mode_option_buf);
 				return 1;
 			}
 
@@ -793,8 +793,10 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 				if (!interlace_specified ||
 				    db_interlace == interlace)
 					if (refresh_specified &&
-					    db[i].refresh == refresh)
+					    db[i].refresh == refresh) {
+						kfree(mode_option_buf);
 						return 1;
+					}
 
 				if (score < diff) {
 					diff = score;
@@ -802,6 +804,8 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 				}
 			}
 		}
+
+		kfree(mode_option_buf);
 		if (best != -1) {
 			fb_try_mode(var, info, &db[best], bpp);
 			return (refresh_specified) ? 2 : 1;
-- 
2.43.0

Re: [PATCH] fbdev:modedb: fix a possible UAF in fb_find_mode()

[Helge Deller]

On 5/26/26 11:15, Tuo Li wrote:
> If mode_option is NULL, it is assigned from mode_option_buf:
> 
>    if (!mode_option) {
>      fb_get_options(NULL, &mode_option_buf);
>      mode_option = mode_option_buf;
>    }
> 
> Later, name is assigned from mode_option:
> 
>    const char *name = mode_option;
> 
> However, mode_option_buf is freed before name is no longer used:
> 
>    kfree(mode_option_buf);
> 
> while name is still accessed by:
> 
>    if ((name_matches(db[i], name, namelen) ||
> 
> Since name aliases mode_option_buf, this may result in a
> use-after-free.
> 
> Fix this by moving the kfree(mode_option_buf) call behind the access, and
> add corresponding cleanup before early returns.

I wonder if this isn't a typical good use-case for the new "Scope-based
resource management for the kernel" [1] feature.

Instead of adding kfree() at various places, we could do:

diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
index 703d0b7aec32..b6926764a99c 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -626,7 +626,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
                  const struct fb_videomode *default_mode,
                  unsigned int default_bpp)
  {
-       char *mode_option_buf = NULL;
+       char *mode_option_buf __free(kfree) = NULL;
         int i;
  
         /* Set up defaults */
@@ -724,7 +724,6 @@ int fb_find_mode(struct fb_var_screeninfo *var,
                         res_specified = 1;
                 }
  done:
-               kfree(mode_option_buf);
                 if (cvt) {
                         struct fb_videomode cvt_mode;
                         int ret;


[1] https://lwn.net/Articles/934679/


Do you want to check if that's correct, and if yes resend a patch?

Helge

Re: [PATCH] fbdev:modedb: fix a possible UAF in fb_find_mode()

[Tuo Li]

Hi Helge,

On Mon, Jun 8, 2026 at 12:12 AM Helge Deller <deller@gmx.de> wrote:
>
> On 5/26/26 11:15, Tuo Li wrote:
> > If mode_option is NULL, it is assigned from mode_option_buf:
> >
> >    if (!mode_option) {
> >      fb_get_options(NULL, &mode_option_buf);
> >      mode_option = mode_option_buf;
> >    }
> >
> > Later, name is assigned from mode_option:
> >
> >    const char *name = mode_option;
> >
> > However, mode_option_buf is freed before name is no longer used:
> >
> >    kfree(mode_option_buf);
> >
> > while name is still accessed by:
> >
> >    if ((name_matches(db[i], name, namelen) ||
> >
> > Since name aliases mode_option_buf, this may result in a
> > use-after-free.
> >
> > Fix this by moving the kfree(mode_option_buf) call behind the access, and
> > add corresponding cleanup before early returns.
>
> I wonder if this isn't a typical good use-case for the new "Scope-based
> resource management for the kernel" [1] feature.
>
> Instead of adding kfree() at various places, we could do:
>
> diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
> index 703d0b7aec32..b6926764a99c 100644
> --- a/drivers/video/fbdev/core/modedb.c
> +++ b/drivers/video/fbdev/core/modedb.c
> @@ -626,7 +626,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
>                   const struct fb_videomode *default_mode,
>                   unsigned int default_bpp)
>   {
> -       char *mode_option_buf = NULL;
> +       char *mode_option_buf __free(kfree) = NULL;
>          int i;
>
>          /* Set up defaults */
> @@ -724,7 +724,6 @@ int fb_find_mode(struct fb_var_screeninfo *var,
>                          res_specified = 1;
>                  }
>   done:
> -               kfree(mode_option_buf);
>                  if (cvt) {
>                          struct fb_videomode cvt_mode;
>                          int ret;
>
>
> [1] https://lwn.net/Articles/934679/
>
>
> Do you want to check if that's correct, and if yes resend a patch?
>
> Helge

Thanks for the suggestion. I think that makes sense. Using __free(kfree)
can simplify the cleanup logic and avoid manually managing multiple kfree()
calls on different return paths.

I'll prepare a v2 based on this approach and send it soon.

Sincerely,
Tuo