| Message ID | 1539981557-13590-1-git-send-email-wang6495@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | video: fbdev: sis: fix a missing-check bug | expand |
Hello, Can anyone please confirm this bug? Thanks! Wenwen On Fri, Oct 19, 2018 at 3:39 PM Wenwen Wang <wang6495@umn.edu> 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@umn.edu> > --- > 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 >
Hi, Sorry for the late reply. On 10/29/2018 08:08 PM, Wenwen Wang wrote: > Hello, > > Can anyone please confirm this bug? Thanks! > > Wenwen > > On Fri, Oct 19, 2018 at 3:39 PM Wenwen Wang <wang6495@umn.edu> 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. The checks in sisfb_check_rom() seem to verify only that the ROM content is valid (== it seems to be a valid code not some random data) by checking few "magic" numbers. There is no checking for the ROM content being safe from security POV of any kind so I fail to see how this patch would help against the malicious device providing insecure ROM content (as it can pass sisfb_check_rom() checks without a problem).. >> 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@umn.edu> >> --- >> 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 Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
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,
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@umn.edu> --- drivers/video/fbdev/sis/sis_main.c | 52 ++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 30 deletions(-)