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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
