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 <[email protected]>
---
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
On 10/18/18 10:37, Wenwen Wang wrote:
> 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.
Thanks for the patch.
However, this IO region is not associated with a hardware device. It is a table
in RAM that is only written to by firmware (coreboot) before Linux is ever run.
So there's no device on the other side that could race with the kernel here.
Regards,
Samuel
> 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 <[email protected]>
> ---
> 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) {
>