mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/2] of: fdt: fix memory leak and oob writes in fdt_ensure_space
@ 2024-01-31 16:56 Stefan Kerkmann
  2024-01-31 16:56 ` [PATCH 1/2] of: fdt: fix memory leak " Stefan Kerkmann
  2024-01-31 16:57 ` [PATCH 2/2] of: fdt: fix oob writes with large ftd properties Stefan Kerkmann
  0 siblings, 2 replies; 10+ messages in thread
From: Stefan Kerkmann @ 2024-01-31 16:56 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX; +Cc: Stefan Kerkmann

I have encountered the oob write while attempting to modify a large FIT
image with of_property. While hunting for the root cause I noticed that
there is a potential memory leak in fdt_ensure_space as well. Both is
fixed in this series.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
Stefan Kerkmann (2):
      of: fdt: fix memory leak in fdt_ensure_space
      of: fdt: fix oob writes with large ftd properties

 drivers/of/fdt.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)
---
base-commit: cecd3fbea3550532d2175bc13aa479a45e605da0
change-id: 20240131-fix-fdt-memory-safety-b06f9164d953

Best regards,
-- 
Stefan Kerkmann <s.kerkmann@pengutronix.de>




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] of: fdt: fix memory leak in fdt_ensure_space
  2024-01-31 16:56 [PATCH 0/2] of: fdt: fix memory leak and oob writes in fdt_ensure_space Stefan Kerkmann
@ 2024-01-31 16:56 ` Stefan Kerkmann
  2024-01-31 17:15   ` Ahmad Fatoum
  2024-01-31 16:57 ` [PATCH 2/2] of: fdt: fix oob writes with large ftd properties Stefan Kerkmann
  1 sibling, 1 reply; 10+ messages in thread
From: Stefan Kerkmann @ 2024-01-31 16:56 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX; +Cc: Stefan Kerkmann

If the reallocation failed the old memory remains allocated and is never
freed, this is fixed by freeing the old memory on error.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 drivers/of/fdt.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 5c21bab5de..4f79a6120f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -375,24 +375,37 @@ static void *memalign_realloc(void *orig, size_t oldsize, size_t newsize)
 
 static int fdt_ensure_space(struct fdt *fdt, int dtsize)
 {
+	size_t new_size;
+	void *previous;
+
 	/*
 	 * We assume strings and names have a maximum length of 1024
 	 * whereas properties can be longer. We allocate new memory
 	 * if we have less than 1024 bytes (+ the property size left.
 	 */
 	if (fdt->str_size - fdt->str_nextofs < 1024) {
-		fdt->strings = realloc(fdt->strings, fdt->str_size * 2);
-		if (!fdt->strings)
+		previous = fdt->strings;
+		new_size = fdt->str_size * 2;
+
+		if ((fdt->strings = realloc(previous, new_size)) == NULL) {
+			free(previous);
 			return -ENOMEM;
-		fdt->str_size *= 2;
+		}
+
+		fdt->str_size = new_size;
 	}
 
 	if (fdt->dt_size - fdt->dt_nextofs < 1024 + dtsize) {
-		fdt->dt = memalign_realloc(fdt->dt, fdt->dt_size,
-				fdt->dt_size * 2);
-		if (!fdt->dt)
+		previous = fdt->dt;
+		new_size = fdt->dt_size * 2;
+
+		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
+						new_size)) == NULL) {
+			free(previous);
 			return -ENOMEM;
-		fdt->dt_size *= 2;
+		}
+
+		fdt->dt_size = new_size;
 	}
 
 	return 0;

-- 
2.39.2




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] of: fdt: fix oob writes with large ftd properties
  2024-01-31 16:56 [PATCH 0/2] of: fdt: fix memory leak and oob writes in fdt_ensure_space Stefan Kerkmann
  2024-01-31 16:56 ` [PATCH 1/2] of: fdt: fix memory leak " Stefan Kerkmann
@ 2024-01-31 16:57 ` Stefan Kerkmann
  2024-01-31 17:21   ` Ahmad Fatoum
  2024-02-01  7:47   ` Sascha Hauer
  1 sibling, 2 replies; 10+ messages in thread
From: Stefan Kerkmann @ 2024-01-31 16:57 UTC (permalink / raw)
  To: Sascha Hauer, BAREBOX; +Cc: Stefan Kerkmann

OOB writes can be triggered when fdt->dt_size * 2 is still smaller than
the property for which memory should be allocated. This can happen under
rare circumstances when editing a fdt with the of_property command and a
property is larger than 128k in size.

This happend when editing a FIT image (which is a ftd) with the
of_property command and the Kernel image was around 8M in size.

The simplified call chain is the following:

of_property -> of_flatten_dtb -> create new fdt with 64k in size (this is
fixed) -> __of_flatten_dtb -> attempt to copy kernel image (8M) ->
fdt_ensure_space -> allocate only 128k for fdt->dt -> memcopy 8M into fdt->dt
buffer -> crash

The fix is to grow fdt->dt to hold at least the new property. The power
of 2 increment is untouched to keep the same behaviour otherwise.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 drivers/of/fdt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4f79a6120f..1f24ed0bbc 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -399,6 +399,10 @@ static int fdt_ensure_space(struct fdt *fdt, int dtsize)
 		previous = fdt->dt;
 		new_size = fdt->dt_size * 2;
 
+		while (new_size <= dtsize) {
+			new_size *= 2;
+		}
+
 		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
 						new_size)) == NULL) {
 			free(previous);

-- 
2.39.2




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] of: fdt: fix memory leak in fdt_ensure_space
  2024-01-31 16:56 ` [PATCH 1/2] of: fdt: fix memory leak " Stefan Kerkmann
@ 2024-01-31 17:15   ` Ahmad Fatoum
  2024-02-01  8:34     ` Stefan Kerkmann
  0 siblings, 1 reply; 10+ messages in thread
From: Ahmad Fatoum @ 2024-01-31 17:15 UTC (permalink / raw)
  To: Stefan Kerkmann, Sascha Hauer, BAREBOX

Hello Stefan,

On 31.01.24 17:56, Stefan Kerkmann wrote:
> If the reallocation failed the old memory remains allocated and is never
> freed, this is fixed by freeing the old memory on error.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>  drivers/of/fdt.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 5c21bab5de..4f79a6120f 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -375,24 +375,37 @@ static void *memalign_realloc(void *orig, size_t oldsize, size_t newsize)
>  
>  static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>  {
> +	size_t new_size;
> +	void *previous;
> +
>  	/*
>  	 * We assume strings and names have a maximum length of 1024
>  	 * whereas properties can be longer. We allocate new memory
>  	 * if we have less than 1024 bytes (+ the property size left.
>  	 */
>  	if (fdt->str_size - fdt->str_nextofs < 1024) {
> -		fdt->strings = realloc(fdt->strings, fdt->str_size * 2);
> -		if (!fdt->strings)
> +		previous = fdt->strings;
> +		new_size = fdt->str_size * 2;
> +
> +		if ((fdt->strings = realloc(previous, new_size)) == NULL) {

IMO, it's less readable this way. I'd prefer we leave the realloc line and
then !fdt->strings check on separate lines as before.

> +			free(previous);

This could happen later in the callers (look for out_free) if one wouldn't
set fdt->strings to NULL on error. I don't feel strongly either way, so doing
it this way is fine too.

Change looks good otherwise, so with first comment addressed:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Cheers,
Ahmad

>  			return -ENOMEM;
> -		fdt->str_size *= 2;
> +		}
> +
> +		fdt->str_size = new_size;
>  	}
>  
>  	if (fdt->dt_size - fdt->dt_nextofs < 1024 + dtsize) {
> -		fdt->dt = memalign_realloc(fdt->dt, fdt->dt_size,
> -				fdt->dt_size * 2);
> -		if (!fdt->dt)
> +		previous = fdt->dt;
> +		new_size = fdt->dt_size * 2;
> +
> +		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
> +						new_size)) == NULL) {
> +			free(previous);
>  			return -ENOMEM;
> -		fdt->dt_size *= 2;
> +		}
> +
> +		fdt->dt_size = new_size;
>  	}
>  
>  	return 0;
> 

-- 
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 |




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] of: fdt: fix oob writes with large ftd properties
  2024-01-31 16:57 ` [PATCH 2/2] of: fdt: fix oob writes with large ftd properties Stefan Kerkmann
@ 2024-01-31 17:21   ` Ahmad Fatoum
  2024-02-01 10:24     ` Stefan Kerkmann
  2024-02-01  7:47   ` Sascha Hauer
  1 sibling, 1 reply; 10+ messages in thread
From: Ahmad Fatoum @ 2024-01-31 17:21 UTC (permalink / raw)
  To: Stefan Kerkmann, Sascha Hauer, BAREBOX

On 31.01.24 17:57, Stefan Kerkmann wrote:
> OOB writes can be triggered when fdt->dt_size * 2 is still smaller than
> the property for which memory should be allocated. This can happen under
> rare circumstances when editing a fdt with the of_property command and a
> property is larger than 128k in size.
> 
> This happend when editing a FIT image (which is a ftd) with the
> of_property command and the Kernel image was around 8M in size.
> 
> The simplified call chain is the following:
> 
> of_property -> of_flatten_dtb -> create new fdt with 64k in size (this is
> fixed) -> __of_flatten_dtb -> attempt to copy kernel image (8M) ->
> fdt_ensure_space -> allocate only 128k for fdt->dt -> memcopy 8M into fdt->dt
> buffer -> crash
> 
> The fix is to grow fdt->dt to hold at least the new property. The power
> of 2 increment is untouched to keep the same behaviour otherwise.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>  drivers/of/fdt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4f79a6120f..1f24ed0bbc 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -399,6 +399,10 @@ static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>  		previous = fdt->dt;
>  		new_size = fdt->dt_size * 2;
>  
> +		while (new_size <= dtsize) {
> +			new_size *= 2;
> +		}

A nitpick that I solely note because I already had feedback on the first patch:
Kernel coding style is to omit { braces } for single statement blocks.

In your case you could just do:

 if (new_size <= dtsize)
 	new_size = roundup_pow_of_two(new_size + dtsize);

I think to skip the loop.

Cheers,
Ahmad

> +
>  		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
>  						new_size)) == NULL) {
>  			free(previous);
> 

-- 
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 |




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] of: fdt: fix oob writes with large ftd properties
  2024-01-31 16:57 ` [PATCH 2/2] of: fdt: fix oob writes with large ftd properties Stefan Kerkmann
  2024-01-31 17:21   ` Ahmad Fatoum
@ 2024-02-01  7:47   ` Sascha Hauer
  2024-02-01  8:49     ` Stefan Kerkmann
  1 sibling, 1 reply; 10+ messages in thread
From: Sascha Hauer @ 2024-02-01  7:47 UTC (permalink / raw)
  To: Stefan Kerkmann; +Cc: BAREBOX


In the subject: s/ftd/fdt/

Sascha

On Wed, Jan 31, 2024 at 05:57:00PM +0100, Stefan Kerkmann wrote:
> OOB writes can be triggered when fdt->dt_size * 2 is still smaller than
> the property for which memory should be allocated. This can happen under
> rare circumstances when editing a fdt with the of_property command and a
> property is larger than 128k in size.
> 
> This happend when editing a FIT image (which is a ftd) with the
> of_property command and the Kernel image was around 8M in size.
> 
> The simplified call chain is the following:
> 
> of_property -> of_flatten_dtb -> create new fdt with 64k in size (this is
> fixed) -> __of_flatten_dtb -> attempt to copy kernel image (8M) ->
> fdt_ensure_space -> allocate only 128k for fdt->dt -> memcopy 8M into fdt->dt
> buffer -> crash
> 
> The fix is to grow fdt->dt to hold at least the new property. The power
> of 2 increment is untouched to keep the same behaviour otherwise.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>  drivers/of/fdt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4f79a6120f..1f24ed0bbc 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -399,6 +399,10 @@ static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>  		previous = fdt->dt;
>  		new_size = fdt->dt_size * 2;
>  
> +		while (new_size <= dtsize) {
> +			new_size *= 2;
> +		}
> +
>  		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
>  						new_size)) == NULL) {
>  			free(previous);
> 
> -- 
> 2.39.2
> 
> 

-- 
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 |



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] of: fdt: fix memory leak in fdt_ensure_space
  2024-01-31 17:15   ` Ahmad Fatoum
@ 2024-02-01  8:34     ` Stefan Kerkmann
  0 siblings, 0 replies; 10+ messages in thread
From: Stefan Kerkmann @ 2024-02-01  8:34 UTC (permalink / raw)
  To: Ahmad Fatoum, Sascha Hauer, BAREBOX

Hello Ahmad,

On 31.01.24 18:15, Ahmad Fatoum wrote:
> Hello Stefan,
> 
> On 31.01.24 17:56, Stefan Kerkmann wrote:
>> If the reallocation failed the old memory remains allocated and is never
>> freed, this is fixed by freeing the old memory on error.
>>
>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>> ---
>>   drivers/of/fdt.c | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 5c21bab5de..4f79a6120f 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -375,24 +375,37 @@ static void *memalign_realloc(void *orig, size_t oldsize, size_t newsize)
>>   
>>   static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>>   {
>> +	size_t new_size;
>> +	void *previous;
>> +
>>   	/*
>>   	 * We assume strings and names have a maximum length of 1024
>>   	 * whereas properties can be longer. We allocate new memory
>>   	 * if we have less than 1024 bytes (+ the property size left.
>>   	 */
>>   	if (fdt->str_size - fdt->str_nextofs < 1024) {
>> -		fdt->strings = realloc(fdt->strings, fdt->str_size * 2);
>> -		if (!fdt->strings)
>> +		previous = fdt->strings;
>> +		new_size = fdt->str_size * 2;
>> +
>> +		if ((fdt->strings = realloc(previous, new_size)) == NULL) {
> 
> IMO, it's less readable this way. I'd prefer we leave the realloc line and
> then !fdt->strings check on separate lines as before.

Applied.

>> +			free(previous);
> 
> This could happen later in the callers (look for out_free) if one wouldn't
> set fdt->strings to NULL on error. I don't feel strongly either way, so doing
> it this way is fine too.
> 

Freeing the previous memory should happen in this function IMHO. In this 
way `fdt->[strings|dt]` can be set to `NULL` and the caller will get a 
segfaulty reminder if they choose to ignore error returned from the 
function and still access `fdt->[strings|dt]`.

> Change looks good otherwise, so with first comment addressed:
> 

Thank you :-).

> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Cheers,
> Ahmad
> 
>>   			return -ENOMEM;
>> -		fdt->str_size *= 2;
>> +		}
>> +
>> +		fdt->str_size = new_size;
>>   	}
>>   
>>   	if (fdt->dt_size - fdt->dt_nextofs < 1024 + dtsize) {
>> -		fdt->dt = memalign_realloc(fdt->dt, fdt->dt_size,
>> -				fdt->dt_size * 2);
>> -		if (!fdt->dt)
>> +		previous = fdt->dt;
>> +		new_size = fdt->dt_size * 2;
>> +
>> +		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
>> +						new_size)) == NULL) {
>> +			free(previous);
>>   			return -ENOMEM;
>> -		fdt->dt_size *= 2;
>> +		}
>> +
>> +		fdt->dt_size = new_size;
>>   	}
>>   
>>   	return 0;
>>
> 

Cheers
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] of: fdt: fix oob writes with large ftd properties
  2024-02-01  7:47   ` Sascha Hauer
@ 2024-02-01  8:49     ` Stefan Kerkmann
  0 siblings, 0 replies; 10+ messages in thread
From: Stefan Kerkmann @ 2024-02-01  8:49 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: BAREBOX

Hi Sascha,

On 01.02.24 08:47, Sascha Hauer wrote:
> 
> In the subject: s/ftd/fdt/
> 

Applied.

> Sascha
> 
> On Wed, Jan 31, 2024 at 05:57:00PM +0100, Stefan Kerkmann wrote:
>> OOB writes can be triggered when fdt->dt_size * 2 is still smaller than
>> the property for which memory should be allocated. This can happen under
>> rare circumstances when editing a fdt with the of_property command and a
>> property is larger than 128k in size.
>>
>> This happend when editing a FIT image (which is a ftd) with the
>> of_property command and the Kernel image was around 8M in size.
>>
>> The simplified call chain is the following:
>>
>> of_property -> of_flatten_dtb -> create new fdt with 64k in size (this is
>> fixed) -> __of_flatten_dtb -> attempt to copy kernel image (8M) ->
>> fdt_ensure_space -> allocate only 128k for fdt->dt -> memcopy 8M into fdt->dt
>> buffer -> crash
>>
>> The fix is to grow fdt->dt to hold at least the new property. The power
>> of 2 increment is untouched to keep the same behaviour otherwise.
>>
>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>> ---
>>   drivers/of/fdt.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 4f79a6120f..1f24ed0bbc 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -399,6 +399,10 @@ static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>>   		previous = fdt->dt;
>>   		new_size = fdt->dt_size * 2;
>>   
>> +		while (new_size <= dtsize) {
>> +			new_size *= 2;
>> +		}
>> +
>>   		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
>>   						new_size)) == NULL) {
>>   			free(previous);
>>
>> -- 
>> 2.39.2
>>
>>
> 

Cheers
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] of: fdt: fix oob writes with large ftd properties
  2024-01-31 17:21   ` Ahmad Fatoum
@ 2024-02-01 10:24     ` Stefan Kerkmann
  2024-02-01 10:42       ` Ahmad Fatoum
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Kerkmann @ 2024-02-01 10:24 UTC (permalink / raw)
  To: Ahmad Fatoum, Sascha Hauer, BAREBOX

Hello Ahmad,

On 31.01.24 18:21, Ahmad Fatoum wrote:
> On 31.01.24 17:57, Stefan Kerkmann wrote:
>> OOB writes can be triggered when fdt->dt_size * 2 is still smaller than
>> the property for which memory should be allocated. This can happen under
>> rare circumstances when editing a fdt with the of_property command and a
>> property is larger than 128k in size.
>>
>> This happend when editing a FIT image (which is a ftd) with the
>> of_property command and the Kernel image was around 8M in size.
>>
>> The simplified call chain is the following:
>>
>> of_property -> of_flatten_dtb -> create new fdt with 64k in size (this is
>> fixed) -> __of_flatten_dtb -> attempt to copy kernel image (8M) ->
>> fdt_ensure_space -> allocate only 128k for fdt->dt -> memcopy 8M into fdt->dt
>> buffer -> crash
>>
>> The fix is to grow fdt->dt to hold at least the new property. The power
>> of 2 increment is untouched to keep the same behaviour otherwise.
>>
>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>> ---
>>   drivers/of/fdt.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 4f79a6120f..1f24ed0bbc 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -399,6 +399,10 @@ static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>>   		previous = fdt->dt;
>>   		new_size = fdt->dt_size * 2;
>>   
>> +		while (new_size <= dtsize) {
>> +			new_size *= 2;
>> +		}
> 
> A nitpick that I solely note because I already had feedback on the first patch:
> Kernel coding style is to omit { braces } for single statement blocks.
> 
> In your case you could just do:
> 
>   if (new_size <= dtsize)
>   	new_size = roundup_pow_of_two(new_size + dtsize);
> 
> I think to skip the loop.
> 

Thanks! That is the better solution.

To not over provision memory I changed the new size to be 
`roundup_pow_of_two(fdt->dt_size + dtsize)` as we know for sure that 
`dtsize` is already larger than `fdt->dt_size * 2`.

In (made up) case that we already have 8k space for the fdt and got a 
17k property we would allocate 65k (8k + 8k + 17k = 33k ⇾ rounded ⇾ 65k) 
and only 32k (8k + 17k = 25k → rounded → 32k) with `fdt->dt_size + dtsize`.

> Cheers,
> Ahmad
> 
>> +
>>   		if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
>>   						new_size)) == NULL) {
>>   			free(previous);
>>
> 

Cheers
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] of: fdt: fix oob writes with large ftd properties
  2024-02-01 10:24     ` Stefan Kerkmann
@ 2024-02-01 10:42       ` Ahmad Fatoum
  0 siblings, 0 replies; 10+ messages in thread
From: Ahmad Fatoum @ 2024-02-01 10:42 UTC (permalink / raw)
  To: Stefan Kerkmann, Sascha Hauer, BAREBOX

On 01.02.24 11:24, Stefan Kerkmann wrote:
> Hello Ahmad,
> 
> On 31.01.24 18:21, Ahmad Fatoum wrote:
>> On 31.01.24 17:57, Stefan Kerkmann wrote:
>>> OOB writes can be triggered when fdt->dt_size * 2 is still smaller than
>>> the property for which memory should be allocated. This can happen under
>>> rare circumstances when editing a fdt with the of_property command and a
>>> property is larger than 128k in size.
>>>
>>> This happend when editing a FIT image (which is a ftd) with the
>>> of_property command and the Kernel image was around 8M in size.
>>>
>>> The simplified call chain is the following:
>>>
>>> of_property -> of_flatten_dtb -> create new fdt with 64k in size (this is
>>> fixed) -> __of_flatten_dtb -> attempt to copy kernel image (8M) ->
>>> fdt_ensure_space -> allocate only 128k for fdt->dt -> memcopy 8M into fdt->dt
>>> buffer -> crash
>>>
>>> The fix is to grow fdt->dt to hold at least the new property. The power
>>> of 2 increment is untouched to keep the same behaviour otherwise.
>>>
>>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>>> ---
>>>   drivers/of/fdt.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 4f79a6120f..1f24ed0bbc 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -399,6 +399,10 @@ static int fdt_ensure_space(struct fdt *fdt, int dtsize)
>>>           previous = fdt->dt;
>>>           new_size = fdt->dt_size * 2;
>>>   +        while (new_size <= dtsize) {
>>> +            new_size *= 2;
>>> +        }
>>
>> A nitpick that I solely note because I already had feedback on the first patch:
>> Kernel coding style is to omit { braces } for single statement blocks.
>>
>> In your case you could just do:
>>
>>   if (new_size <= dtsize)
>>       new_size = roundup_pow_of_two(new_size + dtsize);
>>
>> I think to skip the loop.
>>
> 
> Thanks! That is the better solution.
> 
> To not over provision memory I changed the new size to be `roundup_pow_of_two(fdt->dt_size + dtsize)` as we know for sure that `dtsize` is already larger than `fdt->dt_size * 2`.
> 
> In (made up) case that we already have 8k space for the fdt and got a 17k property we would allocate 65k (8k + 8k + 17k = 33k ⇾ rounded ⇾ 65k) and only 32k (8k + 17k = 25k → rounded → 32k) with `fdt->dt_size + dtsize`.

Sounds good.

> 
>> Cheers,
>> Ahmad
>>
>>> +
>>>           if ((fdt->dt = memalign_realloc(previous, fdt->dt_size,
>>>                           new_size)) == NULL) {
>>>               free(previous);
>>>
>>
> 
> Cheers
> Stefan
> 

-- 
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 |




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-02-01 10:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-31 16:56 [PATCH 0/2] of: fdt: fix memory leak and oob writes in fdt_ensure_space Stefan Kerkmann
2024-01-31 16:56 ` [PATCH 1/2] of: fdt: fix memory leak " Stefan Kerkmann
2024-01-31 17:15   ` Ahmad Fatoum
2024-02-01  8:34     ` Stefan Kerkmann
2024-01-31 16:57 ` [PATCH 2/2] of: fdt: fix oob writes with large ftd properties Stefan Kerkmann
2024-01-31 17:21   ` Ahmad Fatoum
2024-02-01 10:24     ` Stefan Kerkmann
2024-02-01 10:42       ` Ahmad Fatoum
2024-02-01  7:47   ` Sascha Hauer
2024-02-01  8:49     ` Stefan Kerkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox