linux-kernel - [PATCH] firmware: coreboot: 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]

Date:   Thu, 18 Oct 2018 10:37:01 -0500
From:   Wenwen Wang <wang6495@....edu>
To:     Wenwen Wang <wang6495@....edu>
Cc:     Kangjie Lu <kjlu@....edu>, Samuel Holland <samuel@...lland.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org (open list)
Subject: [PATCH] firmware: coreboot: Fix a missing-check bug

In coreboot_table_init(), a for loop is used to copy the entries of the
coreboot table. For each entry, the header of the entry, which is a
structure coreboot_table_entry and includes the size of the entry, is
firstly copied from the IO region 'ptr_entry' to 'entry' through the first
memcpy_fromio(). Then the 'entry.size' is used to allocate the
coreboot_device 'device' through kzalloc(). After 'device' is allocated,
the whole entry, including the header, is then copied to 'device->entry'
through the second memcpy_fromio(). Obviously, the header of the entry is
copied twice here. More importantly, no check is enforced after the second
copy to make sure the two copies obtain the same values. Given that the IO
region can also be accessed by the device, it is possible that
'device->entry.size' is different from 'entry.size' after the second copy,
especially when the device race to modify the size value between these two
copies. This can cause undefined behavior of the kernel and introduce
potential security risk, because 'device->entry.size' is inconsistent with
the actual size of the entry.

This patch rewrites the header of each entry after the second copy, using
the value acquired in the first copy. Through this way, the above issue can
be avoided.

Signed-off-by: Wenwen Wang <wang6495@....edu>
---
 drivers/firmware/google/coreboot_table.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 19db570..20fcd54 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -128,6 +128,7 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
 		device->dev.bus = &coreboot_bus_type;
 		device->dev.release = coreboot_device_release;
 		memcpy_fromio(&device->entry, ptr_entry, entry.size);
+		device->entry = entry;

 		ret = device_register(&device->dev);
 		if (ret) {
-- 
2.7.4