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