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 <[email protected]>
---
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
Hello,
Can anyone please confirm this bug? Thanks!
Wenwen
On Fri, Oct 19, 2018 at 3:39 PM Wenwen Wang <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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