[PATCH master] of: fdt: fix overflows when parsing sizes

[PATCH master] of: fdt: fix overflows when parsing sizes

[Ahmad Fatoum]

The function dt_struct_advance() is used to advance a pointer to the next
offset within the structure block, while checking that the result is in
bounds.

Unfortunately, the function used a signed size argument. This had the
effect that a too-large size in the FDT wrapped around and caused the
pointer to move backwards.

This issue was found by libfuzzer which generated an FDT that
always triggered an out-of-memory condition: One struct indicated a size
that caused the pointer to move backwards.

The resulting loop allocated memory on every iteration and eventually
ran out.

Fix this by using unsigned sizes and treating wrap around as an
error case.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/fdt.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 8dca41990c87..237468cd8164 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -32,12 +32,13 @@ static inline bool __dt_ptr_ok(const struct fdt_header *fdt, const void *p,
 }
 #define dt_ptr_ok(fdt, p) __dt_ptr_ok(fdt, p, sizeof(*(p)), __alignof__(*(p)))
 
-static inline uint32_t dt_struct_advance(struct fdt_header *f, uint32_t dt, int size)
+static inline uint32_t dt_struct_advance(struct fdt_header *f, uint32_t dt, uint32_t size)
 {
-	dt += size;
-	dt = ALIGN(dt, 4);
+	if (check_add_overflow(dt, size, &dt))
+		return 0;
 
-	if (dt > f->off_dt_struct + f->size_dt_struct)
+	dt = ALIGN(dt, 4);
+	if ((!dt && size) || dt > f->off_dt_struct + f->size_dt_struct)
 		return 0;
 
 	return dt;
@@ -165,7 +166,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size,
 {
 	const void *nodep;	/* property node pointer */
 	uint32_t tag;		/* tag */
-	int  len;		/* length of the property */
+	uint32_t len;		/* length of the property */
 	const struct fdt_property *fdt_prop;
 	const char *pathp, *name;
 	struct device_node *root, *node = NULL;
-- 
2.39.2

Re: [PATCH master] of: fdt: fix overflows when parsing sizes

[Sascha Hauer]

On Mon, Jul 22, 2024 at 07:24:10PM +0200, Ahmad Fatoum wrote:
> The function dt_struct_advance() is used to advance a pointer to the next
> offset within the structure block, while checking that the result is in
> bounds.
> 
> Unfortunately, the function used a signed size argument. This had the
> effect that a too-large size in the FDT wrapped around and caused the
> pointer to move backwards.
> 
> This issue was found by libfuzzer which generated an FDT that
> always triggered an out-of-memory condition: One struct indicated a size
> that caused the pointer to move backwards.
> 
> The resulting loop allocated memory on every iteration and eventually
> ran out.
> 
> Fix this by using unsigned sizes and treating wrap around as an
> error case.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/of/fdt.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 8dca41990c87..237468cd8164 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -32,12 +32,13 @@ static inline bool __dt_ptr_ok(const struct fdt_header *fdt, const void *p,
>  }
>  #define dt_ptr_ok(fdt, p) __dt_ptr_ok(fdt, p, sizeof(*(p)), __alignof__(*(p)))
>  
> -static inline uint32_t dt_struct_advance(struct fdt_header *f, uint32_t dt, int size)
> +static inline uint32_t dt_struct_advance(struct fdt_header *f, uint32_t dt, uint32_t size)
>  {
> -	dt += size;
> -	dt = ALIGN(dt, 4);
> +	if (check_add_overflow(dt, size, &dt))
> +		return 0;
>  
> -	if (dt > f->off_dt_struct + f->size_dt_struct)
> +	dt = ALIGN(dt, 4);
> +	if ((!dt && size) || dt > f->off_dt_struct + f->size_dt_struct)
>  		return 0;

I am not sure I fully understand the newly added (!dt && size).

I think it's for the case when the initial addition results in something
like 0xfffffffe and the ALIGN(dt, 4) makes dt become 0, right?

I think dt being zero is a an error anyway, so what is the && size good
for?

>  
>  	return dt;

When dt is zero it is returned here which will be considered an error by
the caller anyway, so it seems the (!dt && size) check doesn't add
anything.

Note we have dt_struct_advance() twice in the tree. Care to fix the
other place as well?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master] of: fdt: fix overflows when parsing sizes

[Ahmad Fatoum]

On 24.07.24 10:36, Sascha Hauer wrote:
> On Mon, Jul 22, 2024 at 07:24:10PM +0200, Ahmad Fatoum wrote:
>> The function dt_struct_advance() is used to advance a pointer to the next
>> offset within the structure block, while checking that the result is in
>> bounds.
>>
>> Unfortunately, the function used a signed size argument. This had the
>> effect that a too-large size in the FDT wrapped around and caused the
>> pointer to move backwards.
>>
>> This issue was found by libfuzzer which generated an FDT that
>> always triggered an out-of-memory condition: One struct indicated a size
>> that caused the pointer to move backwards.
>>
>> The resulting loop allocated memory on every iteration and eventually
>> ran out.
>>
>> Fix this by using unsigned sizes and treating wrap around as an
>> error case.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/of/fdt.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 8dca41990c87..237468cd8164 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -32,12 +32,13 @@ static inline bool __dt_ptr_ok(const struct fdt_header *fdt, const void *p,
>>  }
>>  #define dt_ptr_ok(fdt, p) __dt_ptr_ok(fdt, p, sizeof(*(p)), __alignof__(*(p)))
>>  
>> -static inline uint32_t dt_struct_advance(struct fdt_header *f, uint32_t dt, int size)
>> +static inline uint32_t dt_struct_advance(struct fdt_header *f, uint32_t dt, uint32_t size)
>>  {
>> -	dt += size;
>> -	dt = ALIGN(dt, 4);
>> +	if (check_add_overflow(dt, size, &dt))
>> +		return 0;
>>  
>> -	if (dt > f->off_dt_struct + f->size_dt_struct)
>> +	dt = ALIGN(dt, 4);
>> +	if ((!dt && size) || dt > f->off_dt_struct + f->size_dt_struct)
>>  		return 0;
> 
> I am not sure I fully understand the newly added (!dt && size).
> 
> I think it's for the case when the initial addition results in something
> like 0xfffffffe and the ALIGN(dt, 4) makes dt become 0, right?

Exactly.

> I think dt being zero is a an error anyway, so what is the && size good
> for?

You're right. I will drop the check for v2.

> 
>>  
>>  	return dt;
> 
> When dt is zero it is returned here which will be considered an error by
> the caller anyway, so it seems the (!dt && size) check doesn't add
> anything.
> 
> Note we have dt_struct_advance() twice in the tree. Care to fix the
> other place as well?

Will do.

Thanks,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master] of: fdt: fix overflows when parsing sizes

[Sascha Hauer]

On Mon, 22 Jul 2024 19:24:10 +0200, Ahmad Fatoum wrote:
> The function dt_struct_advance() is used to advance a pointer to the next
> offset within the structure block, while checking that the result is in
> bounds.
> 
> Unfortunately, the function used a signed size argument. This had the
> effect that a too-large size in the FDT wrapped around and caused the
> pointer to move backwards.
> 
> [...]

Applied, thanks!

[1/1] of: fdt: fix overflows when parsing sizes
      https://git.pengutronix.de/cgit/barebox/commit/?id=26683ab9f27f (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>