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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1539981557-13590-1-git-send-email-wang6495@umn.edu>
Date:   Fri, 19 Oct 2018 15:39:17 -0500
From:   Wenwen Wang <wang6495@....edu>
To:     Wenwen Wang <wang6495@....edu>
Cc:     Kangjie Lu <kjlu@....edu>,
        Thomas Winischhofer <thomas@...ischhofer.net>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        dri-devel@...ts.freedesktop.org (open list:FRAMEBUFFER LAYER),
        linux-fbdev@...r.kernel.org (open list:FRAMEBUFFER LAYER),
        linux-kernel@...r.kernel.org (open list)
Subject: [PATCH] video: fbdev: sis: fix a missing-check bug

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@....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