The memcpy() of the data following a coreboot_table_entry couldn't
be evaluated by the compiler under CONFIG_FORTIFY_SOURCE. To make it
easier to reason about, add an explicit flexible array member to struct
coreboot_device so the entire entry can be copied at once. Additionally,
validate the sizes before copying. Avoids this run-time false positive
warning:
memcpy: detected field-spanning write (size 168) of single field "&device->entry" at drivers/firmware/google/coreboot_table.c:103 (size 8)
Reported-by: Paul Menzel <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Cc: Jack Rosenthal <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Julius Werner <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
v2: move flex array to struct coreboot_device (julius)
v1: https://lore.kernel.org/lkml/[email protected]
---
drivers/firmware/google/coreboot_table.c | 9 +++++++--
drivers/firmware/google/coreboot_table.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 2652c396c423..564a3c908838 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -93,14 +93,19 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
for (i = 0; i < header->table_entries; i++) {
entry = ptr_entry;
- device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL);
+ if (entry->size < sizeof(*entry)) {
+ dev_warn(dev, "coreboot table entry too small!\n");
+ return -EINVAL;
+ }
+
+ device = kzalloc(sizeof(device->dev) + entry->size, GFP_KERNEL);
if (!device)
return -ENOMEM;
device->dev.parent = dev;
device->dev.bus = &coreboot_bus_type;
device->dev.release = coreboot_device_release;
- memcpy(&device->entry, ptr_entry, entry->size);
+ memcpy(device->raw, entry, entry->size);
switch (device->entry.tag) {
case LB_TAG_CBMEM_ENTRY:
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 37f4d335a606..d814dca33a08 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -79,6 +79,7 @@ struct coreboot_device {
struct lb_cbmem_ref cbmem_ref;
struct lb_cbmem_entry cbmem_entry;
struct lb_framebuffer framebuffer;
+ DECLARE_FLEX_ARRAY(u8, raw);
};
};
--
2.34.1
On Fri, Jan 6, 2023 at 7:14 PM Kees Cook <[email protected]> wrote:
>
> The memcpy() of the data following a coreboot_table_entry couldn't
> be evaluated by the compiler under CONFIG_FORTIFY_SOURCE. To make it
> easier to reason about, add an explicit flexible array member to struct
> coreboot_device so the entire entry can be copied at once. Additionally,
> validate the sizes before copying. Avoids this run-time false positive
> warning:
>
> memcpy: detected field-spanning write (size 168) of single field "&device->entry" at drivers/firmware/google/coreboot_table.c:103 (size 8)
>
> Reported-by: Paul Menzel <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Cc: Jack Rosenthal <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Julius Werner <[email protected]>
> Cc: Brian Norris <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> v2: move flex array to struct coreboot_device (julius)
> v1: https://lore.kernel.org/lkml/[email protected]
> ---
> drivers/firmware/google/coreboot_table.c | 9 +++++++--
> drivers/firmware/google/coreboot_table.h | 1 +
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index 2652c396c423..564a3c908838 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -93,14 +93,19 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
> for (i = 0; i < header->table_entries; i++) {
> entry = ptr_entry;
>
> - device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL);
> + if (entry->size < sizeof(*entry)) {
> + dev_warn(dev, "coreboot table entry too small!\n");
> + return -EINVAL;
> + }
> +
> + device = kzalloc(sizeof(device->dev) + entry->size, GFP_KERNEL);
> if (!device)
> return -ENOMEM;
>
> device->dev.parent = dev;
> device->dev.bus = &coreboot_bus_type;
> device->dev.release = coreboot_device_release;
> - memcpy(&device->entry, ptr_entry, entry->size);
> + memcpy(device->raw, entry, entry->size);
>
> switch (device->entry.tag) {
> case LB_TAG_CBMEM_ENTRY:
> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index 37f4d335a606..d814dca33a08 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h
> @@ -79,6 +79,7 @@ struct coreboot_device {
> struct lb_cbmem_ref cbmem_ref;
> struct lb_cbmem_entry cbmem_entry;
> struct lb_framebuffer framebuffer;
> + DECLARE_FLEX_ARRAY(u8, raw);
> };
> };
>
> --
> 2.34.1
>
Reviewed-by: Julius Werner <[email protected]>
> - memcpy(&device->entry, ptr_entry, entry->size);
> + memcpy(device->raw, entry, entry->size);
nit: It's a bit odd to change the source pointer from ptr_entry to
entry here. Technically the static analyzer would be within its rights
to give you a warning for that as well, because you're now
"overrunning" the source struct instead of the destination one.
On Mon, Jan 09, 2023 at 04:02:26PM +0100, Julius Werner wrote:
> Reviewed-by: Julius Werner <[email protected]>
>
> > - memcpy(&device->entry, ptr_entry, entry->size);
> > + memcpy(device->raw, entry, entry->size);
>
> nit: It's a bit odd to change the source pointer from ptr_entry to
> entry here. Technically the static analyzer would be within its rights
> to give you a warning for that as well, because you're now
> "overrunning" the source struct instead of the destination one.
True. We've been focused on write overflows, but yeah, since the
location of the flex array changed, I'll switch this back to ptr_entry.
--
Kees Cook