* [PATCH v2] of: fdt: fix possible overflow during parsing of fdt @ 2024-11-12 19:10 Abdelrahman Youssef 2024-11-12 19:56 ` Ahmad Fatoum 0 siblings, 1 reply; 6+ messages in thread From: Abdelrahman Youssef @ 2024-11-12 19:10 UTC (permalink / raw) To: s.hauer; +Cc: barebox, Abdelrahman Youssef While fuzzing, the name marked by FDT_BEGIN_NODE sometimes extends beyond the struct block area, Causing a heap-overflow. Since `maxlen` is an unsigned integer representing the length of name, It can be negative, So it overflows to large numbers, Causing strnlen() to overflow. So we can just change the type of maxlen to signed and check if it's negative. Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com> --- drivers/of/fdt.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 2c3ea31394..d8d8a4922c 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -176,7 +176,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, void *dt_strings; struct fdt_header f; int ret; - unsigned int maxlen; + int maxlen; const struct fdt_header *fdt = infdt; ret = fdt_parse_header(infdt, size, &f); @@ -210,6 +210,11 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, maxlen = (unsigned long)fdt + f.off_dt_struct + f.size_dt_struct - (unsigned long)name; + if (maxlen < 0) { + ret = -ESPIPE; + goto err; + } + len = strnlen(name, maxlen + 1); if (len > maxlen) { ret = -ESPIPE; -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt 2024-11-12 19:10 [PATCH v2] of: fdt: fix possible overflow during parsing of fdt Abdelrahman Youssef @ 2024-11-12 19:56 ` Ahmad Fatoum 2024-11-13 1:13 ` AbdelRahman Yossef 2024-11-13 12:17 ` Sascha Hauer 0 siblings, 2 replies; 6+ messages in thread From: Ahmad Fatoum @ 2024-11-12 19:56 UTC (permalink / raw) To: Abdelrahman Youssef, s.hauer; +Cc: barebox Hello Abdelrahman, Thanks for your patch. On 12.11.24 20:10, Abdelrahman Youssef wrote: > While fuzzing, the name marked by FDT_BEGIN_NODE sometimes extends beyond > the struct block area, Causing a heap-overflow. > > Since `maxlen` is an unsigned integer representing the length of name, > It can be negative, So it overflows to large numbers, Causing strnlen() > to overflow. > > So we can just change the type of maxlen to signed and check if it's negative. > > Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com> > --- Changelog would've been nice. This also should have been v3 not v2. > drivers/of/fdt.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 2c3ea31394..d8d8a4922c 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -176,7 +176,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > void *dt_strings; > struct fdt_header f; > int ret; > - unsigned int maxlen; > + int maxlen; > const struct fdt_header *fdt = infdt; > > ret = fdt_parse_header(infdt, size, &f); > @@ -210,6 +210,11 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > maxlen = (unsigned long)fdt + f.off_dt_struct + > f.size_dt_struct - (unsigned long)name; > > + if (maxlen < 0) { > + ret = -ESPIPE; > + goto err; > + } > + > len = strnlen(name, maxlen + 1); Hmm is this + 1 correct? I am wondering if we should be dropping the + 1 here and make it maxlen <= 0 above. What do you think? Cheers, Ahmad > if (len > maxlen) {> ret = -ESPIPE; -- 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] 6+ messages in thread
* Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt 2024-11-12 19:56 ` Ahmad Fatoum @ 2024-11-13 1:13 ` AbdelRahman Yossef 2024-11-13 12:17 ` Sascha Hauer 1 sibling, 0 replies; 6+ messages in thread From: AbdelRahman Yossef @ 2024-11-13 1:13 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox Hi, > Changelog would've been nice. This also should have been v3 not v2 So should I write a new patch (v4) and add Changelog? > Hmm is this + 1 correct? I am wondering if we should be dropping > the + 1 here and make it maxlen <= 0 above. > > What do you think? Well, I think the + 1 is unnecessary here. But it's been there for over 11 years, So maybe someone has another opinion on the matter. Cheers, Abdelrahman On Tue, Nov 12, 2024 at 9:56 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: > > Hello Abdelrahman, > > Thanks for your patch. > > On 12.11.24 20:10, Abdelrahman Youssef wrote: > > While fuzzing, the name marked by FDT_BEGIN_NODE sometimes extends beyond > > the struct block area, Causing a heap-overflow. > > > > Since `maxlen` is an unsigned integer representing the length of name, > > It can be negative, So it overflows to large numbers, Causing strnlen() > > to overflow. > > > > So we can just change the type of maxlen to signed and check if it's negative. > > > > Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com> > > --- > > Changelog would've been nice. This also should have been v3 not v2. > > > drivers/of/fdt.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > > index 2c3ea31394..d8d8a4922c 100644 > > --- a/drivers/of/fdt.c > > +++ b/drivers/of/fdt.c > > @@ -176,7 +176,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > > void *dt_strings; > > struct fdt_header f; > > int ret; > > - unsigned int maxlen; > > + int maxlen; > > const struct fdt_header *fdt = infdt; > > > > ret = fdt_parse_header(infdt, size, &f); > > @@ -210,6 +210,11 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > > maxlen = (unsigned long)fdt + f.off_dt_struct + > > f.size_dt_struct - (unsigned long)name; > > > > + if (maxlen < 0) { > > + ret = -ESPIPE; > > + goto err; > > + } > > + > > len = strnlen(name, maxlen + 1); > > Hmm is this + 1 correct? I am wondering if we should be dropping > the + 1 here and make it maxlen <= 0 above. > > What do you think? > > Cheers, > Ahmad > > > if (len > maxlen) {> ret = -ESPIPE; > > > -- > 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] 6+ messages in thread
* Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt 2024-11-12 19:56 ` Ahmad Fatoum 2024-11-13 1:13 ` AbdelRahman Yossef @ 2024-11-13 12:17 ` Sascha Hauer 2024-11-13 12:37 ` AbdelRahman Yossef 1 sibling, 1 reply; 6+ messages in thread From: Sascha Hauer @ 2024-11-13 12:17 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: Abdelrahman Youssef, barebox On Tue, Nov 12, 2024 at 08:56:58PM +0100, Ahmad Fatoum wrote: > Hello Abdelrahman, > > Thanks for your patch. > > On 12.11.24 20:10, Abdelrahman Youssef wrote: > > While fuzzing, the name marked by FDT_BEGIN_NODE sometimes extends beyond > > the struct block area, Causing a heap-overflow. > > > > Since `maxlen` is an unsigned integer representing the length of name, > > It can be negative, So it overflows to large numbers, Causing strnlen() > > to overflow. > > > > So we can just change the type of maxlen to signed and check if it's negative. > > > > Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com> > > --- > > Changelog would've been nice. This also should have been v3 not v2. > > > drivers/of/fdt.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > > index 2c3ea31394..d8d8a4922c 100644 > > --- a/drivers/of/fdt.c > > +++ b/drivers/of/fdt.c > > @@ -176,7 +176,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > > void *dt_strings; > > struct fdt_header f; > > int ret; > > - unsigned int maxlen; > > + int maxlen; > > const struct fdt_header *fdt = infdt; > > > > ret = fdt_parse_header(infdt, size, &f); > > @@ -210,6 +210,11 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > > maxlen = (unsigned long)fdt + f.off_dt_struct + > > f.size_dt_struct - (unsigned long)name; > > > > + if (maxlen < 0) { > > + ret = -ESPIPE; > > + goto err; > > + } > > + > > len = strnlen(name, maxlen + 1); > > Hmm is this + 1 correct? I am wondering if we should be dropping > the + 1 here and make it maxlen <= 0 above. I think maxlen <= 0 is correct indepent of what follows next, because maxlen is the length of a string and a valid string has a minimal length of one byte ('\0'). Next we shouldn't look at bytes exceeding maxlen, so indeed strnlen(name, maxlen) should be correct. When changing this we have to adjust the following if (len > maxlen) check to >=. 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 | ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt 2024-11-13 12:17 ` Sascha Hauer @ 2024-11-13 12:37 ` AbdelRahman Yossef 2024-11-13 17:46 ` Ahmad Fatoum 0 siblings, 1 reply; 6+ messages in thread From: AbdelRahman Yossef @ 2024-11-13 12:37 UTC (permalink / raw) To: Sascha Hauer; +Cc: Ahmad Fatoum, barebox I will update the patch and send it as v4. Is it enough to just add the changes to Changelog or change the commit message? On Wed, Nov 13, 2024 at 2:17 PM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > On Tue, Nov 12, 2024 at 08:56:58PM +0100, Ahmad Fatoum wrote: > > Hello Abdelrahman, > > > > Thanks for your patch. > > > > On 12.11.24 20:10, Abdelrahman Youssef wrote: > > > While fuzzing, the name marked by FDT_BEGIN_NODE sometimes extends beyond > > > the struct block area, Causing a heap-overflow. > > > > > > Since `maxlen` is an unsigned integer representing the length of name, > > > It can be negative, So it overflows to large numbers, Causing strnlen() > > > to overflow. > > > > > > So we can just change the type of maxlen to signed and check if it's negative. > > > > > > Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com> > > > --- > > > > Changelog would've been nice. This also should have been v3 not v2. > > > > > drivers/of/fdt.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > > > index 2c3ea31394..d8d8a4922c 100644 > > > --- a/drivers/of/fdt.c > > > +++ b/drivers/of/fdt.c > > > @@ -176,7 +176,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > > > void *dt_strings; > > > struct fdt_header f; > > > int ret; > > > - unsigned int maxlen; > > > + int maxlen; > > > const struct fdt_header *fdt = infdt; > > > > > > ret = fdt_parse_header(infdt, size, &f); > > > @@ -210,6 +210,11 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, > > > maxlen = (unsigned long)fdt + f.off_dt_struct + > > > f.size_dt_struct - (unsigned long)name; > > > > > > + if (maxlen < 0) { > > > + ret = -ESPIPE; > > > + goto err; > > > + } > > > + > > > len = strnlen(name, maxlen + 1); > > > > Hmm is this + 1 correct? I am wondering if we should be dropping > > the + 1 here and make it maxlen <= 0 above. > > I think maxlen <= 0 is correct indepent of what follows next, because > maxlen is the length of a string and a valid string has a minimal length > of one byte ('\0'). > > Next we shouldn't look at bytes exceeding maxlen, so indeed > strnlen(name, maxlen) should be correct. When changing this we have > to adjust the following if (len > maxlen) check to >=. > > 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 | ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt 2024-11-13 12:37 ` AbdelRahman Yossef @ 2024-11-13 17:46 ` Ahmad Fatoum 0 siblings, 0 replies; 6+ messages in thread From: Ahmad Fatoum @ 2024-11-13 17:46 UTC (permalink / raw) To: AbdelRahman Yossef, Sascha Hauer; +Cc: barebox Hi, On 13.11.24 13:37, AbdelRahman Yossef wrote: > I will update the patch and send it as v4. > > Is it enough to just add the changes to Changelog or change the commit message? The old commit message wouldn't reflect the new changes, so please rewrite it to be in-line with the diff. The changelog is separate and should look like this or similar: --- v3 -> v4: - replace < 0 with <= 0 (Sascha) - remove + 1 in strnlen (Sascha) v2 -> v3: - did foo to bar ($name_of_person_who_suggested_it) v1 > v2: - did baz to bazzer --- Thanks, Ahmad > > On Wed, Nov 13, 2024 at 2:17 PM Sascha Hauer <s.hauer@pengutronix.de> wrote: >> >> On Tue, Nov 12, 2024 at 08:56:58PM +0100, Ahmad Fatoum wrote: >>> Hello Abdelrahman, >>> >>> Thanks for your patch. >>> >>> On 12.11.24 20:10, Abdelrahman Youssef wrote: >>>> While fuzzing, the name marked by FDT_BEGIN_NODE sometimes extends beyond >>>> the struct block area, Causing a heap-overflow. >>>> >>>> Since `maxlen` is an unsigned integer representing the length of name, >>>> It can be negative, So it overflows to large numbers, Causing strnlen() >>>> to overflow. >>>> >>>> So we can just change the type of maxlen to signed and check if it's negative. >>>> >>>> Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com> >>>> --- >>> >>> Changelog would've been nice. This also should have been v3 not v2. >>> >>>> drivers/of/fdt.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >>>> index 2c3ea31394..d8d8a4922c 100644 >>>> --- a/drivers/of/fdt.c >>>> +++ b/drivers/of/fdt.c >>>> @@ -176,7 +176,7 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, >>>> void *dt_strings; >>>> struct fdt_header f; >>>> int ret; >>>> - unsigned int maxlen; >>>> + int maxlen; >>>> const struct fdt_header *fdt = infdt; >>>> >>>> ret = fdt_parse_header(infdt, size, &f); >>>> @@ -210,6 +210,11 @@ static struct device_node *__of_unflatten_dtb(const void *infdt, int size, >>>> maxlen = (unsigned long)fdt + f.off_dt_struct + >>>> f.size_dt_struct - (unsigned long)name; >>>> >>>> + if (maxlen < 0) { >>>> + ret = -ESPIPE; >>>> + goto err; >>>> + } >>>> + >>>> len = strnlen(name, maxlen + 1); >>> >>> Hmm is this + 1 correct? I am wondering if we should be dropping >>> the + 1 here and make it maxlen <= 0 above. >> >> I think maxlen <= 0 is correct indepent of what follows next, because >> maxlen is the length of a string and a valid string has a minimal length >> of one byte ('\0'). >> >> Next we shouldn't look at bytes exceeding maxlen, so indeed >> strnlen(name, maxlen) should be correct. When changing this we have >> to adjust the following if (len > maxlen) check to >=. >> >> 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 | > -- 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] 6+ messages in thread
end of thread, other threads:[~2024-11-13 17:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-11-12 19:10 [PATCH v2] of: fdt: fix possible overflow during parsing of fdt Abdelrahman Youssef 2024-11-12 19:56 ` Ahmad Fatoum 2024-11-13 1:13 ` AbdelRahman Yossef 2024-11-13 12:17 ` Sascha Hauer 2024-11-13 12:37 ` AbdelRahman Yossef 2024-11-13 17:46 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox