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