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

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |