Re: [PATCH] video: fbdev: sis: fix a missing-check bug

[Wenwen Wang]

Hello,

Can anyone please confirm this bug? Thanks!

Wenwen

On Fri, Oct 19, 2018 at 3:39 PM Wenwen Wang <wang6495@xxxxxxx> wrote:
>
> In sisfb_find_rom(), the official pci ROM is checked to see whether it
> contains the expected value at specific locations. This is done by firstly
> mapping the rom into the IO memory region 'rom_base' and then invoking
> sisfb_check_rom() to perform the checks. If the checks succeed, i.e.,
> sisfb_check_rom() returns 1, the whole content of the rom is then copied to
> 'myrombase' through memcpy_fromio(). The problem here is that the checks
> are conducted on the IO region 'rom_base' directly. Given that the device
> also has the permission to access the IO region, it is possible that a
> malicious device controlled by an attacker can race to modify the values in
> the region after the checks but before memcpy_fromio(). By doing so, the
> attacker can supply unexpected roms, which can cause undefined behavior of
> the kernel and introduce potential security risk. The following for loop
> also has a similar issue.
>
> To avoid the above issue, this patch firstly copies the content of the rom
> and then performs the checks on the copied version. If all the checks are
> satisfied, the copied version will then be returned.
>
> Signed-off-by: Wenwen Wang <wang6495@xxxxxxx>
> ---
>  drivers/video/fbdev/sis/sis_main.c | 52 ++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/video/fbdev/sis/sis_main.c b/drivers/video/fbdev/sis/sis_main.c
> index 20aff90..a2d8051 100644
> --- a/drivers/video/fbdev/sis/sis_main.c
> +++ b/drivers/video/fbdev/sis/sis_main.c
> @@ -4089,29 +4089,29 @@ static int __init sisfb_setup(char *options)
>  }
>  #endif
>
> -static int sisfb_check_rom(void __iomem *rom_base,
> +static int sisfb_check_rom(unsigned char *rom_base,
>                            struct sis_video_info *ivideo)
>  {
> -       void __iomem *rom;
> +       unsigned char *rom;
>         int romptr;
>
> -       if((readb(rom_base) != 0x55) || (readb(rom_base + 1) != 0xaa))
> +       if ((*rom_base != 0x55) || (*(rom_base + 1) != 0xaa))
>                 return 0;
>
> -       romptr = (readb(rom_base + 0x18) | (readb(rom_base + 0x19) << 8));
> +       romptr = (*(rom_base + 0x18) | (*(rom_base + 0x19) << 8));
>         if(romptr > (0x10000 - 8))
>                 return 0;
>
>         rom = rom_base + romptr;
>
> -       if((readb(rom)     != 'P') || (readb(rom + 1) != 'C') ||
> -          (readb(rom + 2) != 'I') || (readb(rom + 3) != 'R'))
> +       if ((*(rom)     != 'P') || (*(rom + 1) != 'C') ||
> +          (*(rom + 2) != 'I') || (*(rom + 3) != 'R'))
>                 return 0;
>
> -       if((readb(rom + 4) | (readb(rom + 5) << 8)) != ivideo->chip_vendor)
> +       if ((*(rom + 4) | (*(rom + 5) << 8)) != ivideo->chip_vendor)
>                 return 0;
>
> -       if((readb(rom + 6) | (readb(rom + 7) << 8)) != ivideo->chip_id)
> +       if ((*(rom + 6) | (*(rom + 7) << 8)) != ivideo->chip_id)
>                 return 0;
>
>         return 1;
> @@ -4124,6 +4124,10 @@ static unsigned char *sisfb_find_rom(struct pci_dev *pdev)
>         unsigned char *myrombase = NULL;
>         size_t romsize;
>
> +       myrombase = vmalloc(65536);
> +       if (!myrombase)
> +               return NULL;
> +
>         /* First, try the official pci ROM functions (except
>          * on integrated chipsets which have no ROM).
>          */
> @@ -4131,20 +4135,15 @@ static unsigned char *sisfb_find_rom(struct pci_dev *pdev)
>         if(!ivideo->nbridge) {
>
>                 if((rom_base = pci_map_rom(pdev, &romsize))) {
> -
> -                       if(sisfb_check_rom(rom_base, ivideo)) {
> -
> -                               if((myrombase = vmalloc(65536))) {
> -                                       memcpy_fromio(myrombase, rom_base,
> -                                                       (romsize > 65536) ? 65536 : romsize);
> -                               }
> -                       }
> +                       memcpy_fromio(myrombase, rom_base,
> +                                       (romsize > 65536) ? 65536 : romsize);
>                         pci_unmap_rom(pdev, rom_base);
> +
> +                       if (sisfb_check_rom(myrombase, ivideo))
> +                               return myrombase;
>                 }
>         }
>
> -       if(myrombase) return myrombase;
> -
>         /* Otherwise do it the conventional way. */
>
>  #if defined(__i386__) || defined(__x86_64__)
> @@ -4156,24 +4155,17 @@ static unsigned char *sisfb_find_rom(struct pci_dev *pdev)
>                         rom_base = ioremap(temp, 65536);
>                         if (!rom_base)
>                                 continue;
> -
> -                       if (!sisfb_check_rom(rom_base, ivideo)) {
> -                               iounmap(rom_base);
> -                               continue;
> -                       }
> -
> -                       if ((myrombase = vmalloc(65536)))
> -                               memcpy_fromio(myrombase, rom_base, 65536);
> -
> +                       memcpy_fromio(myrombase, rom_base, 65536);
>                         iounmap(rom_base);
> -                       break;
>
> +                       if (sisfb_check_rom(myrombase, ivideo))
> +                               return myrombase;
>                 }
>
>         }
>  #endif
> -
> -       return myrombase;
> +       vfree(myrombase);
> +       return NULL;
>  }
>
>  static void sisfb_post_map_vram(struct sis_video_info *ivideo,
> --
> 2.7.4
>