[PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[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

Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[Ahmad Fatoum]

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 |

Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[AbdelRahman Yossef]

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 |

Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[Sascha Hauer]

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 |

Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[AbdelRahman Yossef]

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 |

Re: [PATCH v2] of: fdt: fix possible overflow during parsing of fdt

[Ahmad Fatoum]

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 |