[bug report] out of bounds read parsing vmode commandline option

This bug predates git but it looks like it might be simple to fix if the
right person looked at the code.

	drivers/video/fbdev/controlfb.c:560 control_setup()
	error: buffer overflow 'control_mac_modes' 20 <= 21

drivers/video/fbdev/controlfb.c
   549  static void __init control_setup(char *options)
   550  {
   551          char *this_opt;
   552  
   553          if (!options || !*options)
   554                  return;
   555  
   556          while ((this_opt = strsep(&options, ",")) != NULL) {
   557                  if (!strncmp(this_opt, "vmode:", 6)) {
   558                          int vmode = simple_strtoul(this_opt+6, NULL, 0);
                                    ^^^^^
We get vmode from the command line.

   559                          if (vmode > 0 && vmode <= VMODE_MAX &&
                                                          ^^^^^^^^^
We check that it's <= 22.

   560                              control_mac_modes[vmode - 1].m[1] >= 0)
                                    ^^^^^^^^^^^^^^^^^
But the problem is that control_mac_modes[] only has 20 elements so the
highest valid index is 19.  vmode - 1 can be up to 21.

   561                                  default_vmode = vmode;
   562                  } else if (!strncmp(this_opt, "cmode:", 6)) {
   563                          int depth = simple_strtoul(this_opt+6, NULL, 0);
   564                          switch (depth) {
   565                           case CMODE_8:
   566                           case CMODE_16:
   567                           case CMODE_32:
   568                                  default_cmode = depth;
   569                                  break;
   570                           case 8:
   571                                  default_cmode = CMODE_8;
   572                                  break;
   573                           case 15:
   574                           case 16:
   575                                  default_cmode = CMODE_16;
   576                                  break;
   577                           case 24:
   578                           case 32:
   579                                  default_cmode = CMODE_32;
   580                                  break;
   581                          }
   582                  }
   583          }
   584  }

regards,
dan carpenter

Hi Dan,

On Wed, Oct 4, 2017 at 2:50 PM, Dan Carpenter [off-list ref] wrote:

This bug predates git but it looks like it might be simple to fix if the
right person looked at the code.

        drivers/video/fbdev/controlfb.c:560 control_setup()
        error: buffer overflow 'control_mac_modes' 20 <= 21

drivers/video/fbdev/controlfb.c
   549  static void __init control_setup(char *options)
   550  {
   551          char *this_opt;
   552
   553          if (!options || !*options)
   554                  return;
   555
   556          while ((this_opt = strsep(&options, ",")) != NULL) {
   557                  if (!strncmp(this_opt, "vmode:", 6)) {
   558                          int vmode = simple_strtoul(this_opt+6, NULL, 0);
                                    ^^^^^
We get vmode from the command line.

   559                          if (vmode > 0 && vmode <= VMODE_MAX &&
                                                          ^^^^^^^^^
We check that it's <= 22.

   560                              control_mac_modes[vmode - 1].m[1] >= 0)
                                    ^^^^^^^^^^^^^^^^^
But the problem is that control_mac_modes[] only has 20 elements so the
highest valid index is 19.  vmode - 1 can be up to 21.

Nice catch!

The bug was introduced in v2.4.5.6, when 2 new modes were added to
macmodes.h, but control_mac_modes[] wasn't updated:

https://kernel.opensuse.org/cgit/kernel/diff/include/video/macmodes.h?h=v2.5.2&id)f279c764808560eaceb88fef36cbc35c529aad

A simple fix is to check against ARRAY_SIZE(control_mac_modes) instead.

A better fix is to add the missing entries to control_mac_modes[], cfr. the
(gmail-whitespace-damaged) patch below:

--- a/drivers/video/fbdev/controlfb.h
+++ b/drivers/video/fbdev/controlfb.h

@@ -141,5 +141,7 @@ static struct max_cmodes control_mac_modes[] = {
        {{ 1, 2}},      /* 1152x870, 75Hz */
        {{ 0, 1}},      /* 1280x960, 75Hz */
        {{ 0, 1}},      /* 1280x1024, 75Hz */
+       {{ 1, 2}},      /* 1152x768, 60Hz */
+       {{ 0, 1}},      /* 1600x1024, 60Hz */
 };

(this array lists the maximum color mode (8, 16, or 32 bpp) for each
 video mode given RAM restrictions (2 or 4 MiB)).

The 1152x768 mode is probably OK. Given the 1600x1024 mode has a lower
dotclock (112 MHz) than the supported 1280x960 mode, it's probably OK, too.

platinum_reg_init[] and valkyrie_reg_init[] seem to be handled fine.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hi,

On Wednesday, October 04, 2017 06:42:37 PM Geert Uytterhoeven wrote:

Looks fine to me, could you please submit it as a normal patch?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics