2022-05-04 01:58:58

by Kees Cook

Subject: [PATCH 29/32] xtensa: Use mem_to_flex_dup() with struct property

As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.

Cc: Chris Zankel <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
arch/xtensa/platforms/xtfpga/setup.c | 9 +++------
include/linux/of.h | 3 ++-
2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/xtensa/platforms/xtfpga/setup.c b/arch/xtensa/platforms/xtfpga/setup.c
index 538e6748e85a..31c1fa4ba4ec 100644
--- a/arch/xtensa/platforms/xtfpga/setup.c
+++ b/arch/xtensa/platforms/xtfpga/setup.c
@@ -102,7 +102,7 @@ CLK_OF_DECLARE(xtfpga_clk, "cdns,xtfpga-clock", xtfpga_clk_setup);
#define MAC_LEN 6
static void __init update_local_mac(struct device_node *node)
{
- struct property *newmac;
+ struct property *newmac = NULL;
const u8* macaddr;
int prop_len;

@@ -110,19 +110,16 @@ static void __init update_local_mac(struct device_node *node)
if (macaddr == NULL || prop_len != MAC_LEN)
return;

- newmac = kzalloc(sizeof(*newmac) + MAC_LEN, GFP_KERNEL);
- if (newmac == NULL)
+ if (mem_to_flex_dup(&newmac, macaddr, MAC_LEN, GFP_KERNEL))
return;

- newmac->value = newmac + 1;
- newmac->length = MAC_LEN;
+ newmac->value = newmac->contents;
newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
if (newmac->name == NULL) {
kfree(newmac);
return;
}

- memcpy(newmac->value, macaddr, MAC_LEN);
((u8*)newmac->value)[5] = (*(u32*)DIP_SWITCHES_VADDR) & 0x3f;
of_update_property(node, newmac);
}
diff --git a/include/linux/of.h b/include/linux/of.h
index 17741eee0ca4..efb0f419fd1f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -30,7 +30,7 @@ typedef u32 ihandle;

struct property {
char *name;
- int length;
+ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, length);
void *value;
struct property *next;
#if defined(CONFIG_OF_DYNAMIC) || defined(CONFIG_SPARC)
@@ -42,6 +42,7 @@ struct property {
#if defined(CONFIG_OF_KOBJ)
struct bin_attribute attr;
#endif
+ DECLARE_FLEX_ARRAY_ELEMENTS(u8, contents);
};

#if defined(CONFIG_SPARC)
--
2.32.0

2022-05-04 20:55:16

by Rob Herring

Subject: Re: [PATCH 29/32] xtensa: Use mem_to_flex_dup() with struct property

Gmail won't send this, so I've trimmed the recipients...

On Tue, May 03, 2022 at 06:44:38PM -0700, Kees Cook wrote:
> As part of the work to perform bounds checking on all memcpy() uses,
> replace the open-coded a deserialization of bytes out of memory into a
> trailing flexible array by using a flex_array.h helper to perform the
> allocation, bounds checking, and copying.
>
> Cc: Chris Zankel <[email protected]>
> Cc: Max Filippov <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> arch/xtensa/platforms/xtfpga/setup.c | 9 +++------
> include/linux/of.h | 3 ++-
> 2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/xtensa/platforms/xtfpga/setup.c b/arch/xtensa/platforms/xtfpga/setup.c
> index 538e6748e85a..31c1fa4ba4ec 100644
> --- a/arch/xtensa/platforms/xtfpga/setup.c
> +++ b/arch/xtensa/platforms/xtfpga/setup.c
> @@ -102,7 +102,7 @@ CLK_OF_DECLARE(xtfpga_clk, "cdns,xtfpga-clock", xtfpga_clk_setup);
> #define MAC_LEN 6
> static void __init update_local_mac(struct device_node *node)
> {
> - struct property *newmac;
> + struct property *newmac = NULL;
> const u8* macaddr;
> int prop_len;
>
> @@ -110,19 +110,16 @@ static void __init update_local_mac(struct device_node *node)
> if (macaddr == NULL || prop_len != MAC_LEN)
> return;
>
> - newmac = kzalloc(sizeof(*newmac) + MAC_LEN, GFP_KERNEL);
> - if (newmac == NULL)
> + if (mem_to_flex_dup(&newmac, macaddr, MAC_LEN, GFP_KERNEL))
> return;
>
> - newmac->value = newmac + 1;
> - newmac->length = MAC_LEN;
> + newmac->value = newmac->contents;
> newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
> if (newmac->name == NULL) {
> kfree(newmac);
> return;
> }
>
> - memcpy(newmac->value, macaddr, MAC_LEN);
> ((u8*)newmac->value)[5] = (*(u32*)DIP_SWITCHES_VADDR) & 0x3f;
> of_update_property(node, newmac);
> }
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 17741eee0ca4..efb0f419fd1f 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -30,7 +30,7 @@ typedef u32 ihandle;
>
> struct property {
> char *name;
> - int length;
> + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, length);
> void *value;
> struct property *next;
> #if defined(CONFIG_OF_DYNAMIC) || defined(CONFIG_SPARC)
> @@ -42,6 +42,7 @@ struct property {
> #if defined(CONFIG_OF_KOBJ)
> struct bin_attribute attr;
> #endif
> + DECLARE_FLEX_ARRAY_ELEMENTS(u8, contents);

99.9% of the time, this is not where the property value is stored as it
points into an FDT blob. I suppose that is okay, but just want to make
sure.

The DT API for creating new nodes and properties is horrible as it is
multiple allocs and strdups which makes for tricky error paths. A better
API to centralize it would be welcome, but if this is the only case you
came across it's certainly not a requirement.

Rob