[PATCH] libfdt: fix undefined behaviour in fdt_splice_()

[PATCH] libfdt: fix undefined behaviour in fdt_splice_()

[Jan Beulich]

libfdt: fix undefined behaviour in fdt_splice_()

Along the lines of commit d0b3ab0a0f46 ("libfdt: Fix undefined behaviour
in fdt_offset_ptr()"), fdt_splice_() similarly may not use pointer
arithmetic to do overflow checks. (The left side of the checks added by
d4c7c25c9ed1 ["libfdt: check for potential overrun in _fdt_splice()"]
doesn't really lend itself to similar replacement though.)

Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
---
The right side of the checks added by d4c7c25c9ed1 looks suspicious,
which is more noticable after the transformation done here:

	end - oldlen + newlen < (char *)fdt

	end - (char *)fdt + newlen < oldlen

	dsize + newlen < oldlen

--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -46,7 +46,7 @@ static int fdt_rw_probe_(void *fdt)
 			return err_; \
 	}
 
-static inline int fdt_data_size_(void *fdt)
+static inline unsigned int fdt_data_size_(void *fdt)
 {
 	return fdt_off_dt_strings(fdt) + fdt_size_dt_strings(fdt);
 }
@@ -54,15 +54,16 @@ static inline int fdt_data_size_(void *f
 static int fdt_splice_(void *fdt, void *splicepoint, int oldlen, int newlen)
 {
 	char *p = splicepoint;
-	char *end = (char *)fdt + fdt_data_size_(fdt);
+	unsigned int dsize = fdt_data_size_(fdt);
+	size_t soff = p - (char *)fdt;
 
-	if (((p + oldlen) < p) || ((p + oldlen) > end))
+	if ((oldlen < 0) || (soff + oldlen < soff) || (soff + oldlen > dsize))
 		return -FDT_ERR_BADOFFSET;
-	if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+	if ((p < (char *)fdt) || (dsize + newlen < oldlen))
 		return -FDT_ERR_BADOFFSET;
-	if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
+	if (dsize - oldlen + newlen > fdt_totalsize(fdt))
 		return -FDT_ERR_NOSPACE;
-	memmove(p + newlen, p + oldlen, end - p - oldlen);
+	memmove(p + newlen, p + oldlen, ((char *)fdt + dsize) - (p + oldlen));
 	return 0;
 }
 

Re: [PATCH] libfdt: fix undefined behaviour in fdt_splice_()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

On Thu, Mar 05, 2020 at 03:04:45PM +0100, Jan Beulich wrote:
> libfdt: fix undefined behaviour in fdt_splice_()
> 
> Along the lines of commit d0b3ab0a0f46 ("libfdt: Fix undefined behaviour
> in fdt_offset_ptr()"), fdt_splice_() similarly may not use pointer
> arithmetic to do overflow checks. (The left side of the checks added by
> d4c7c25c9ed1 ["libfdt: check for potential overrun in _fdt_splice()"]
> doesn't really lend itself to similar replacement though.)
> 
> Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>

Applied, thanks.

> ---
> The right side of the checks added by d4c7c25c9ed1 looks suspicious,
> which is more noticable after the transformation done here:
> 
> 	end - oldlen + newlen < (char *)fdt
> 
> 	end - (char *)fdt + newlen < oldlen
> 
> 	dsize + newlen < oldlen

Hrm, yeah, I think this is trying to check for overflow of
dsize+newlen, but it's pretty hard to follow.

> 
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -46,7 +46,7 @@ static int fdt_rw_probe_(void *fdt)
>  			return err_; \
>  	}
>  
> -static inline int fdt_data_size_(void *fdt)
> +static inline unsigned int fdt_data_size_(void *fdt)
>  {
>  	return fdt_off_dt_strings(fdt) + fdt_size_dt_strings(fdt);
>  }
> @@ -54,15 +54,16 @@ static inline int fdt_data_size_(void *f
>  static int fdt_splice_(void *fdt, void *splicepoint, int oldlen, int newlen)
>  {
>  	char *p = splicepoint;
> -	char *end = (char *)fdt + fdt_data_size_(fdt);
> +	unsigned int dsize = fdt_data_size_(fdt);
> +	size_t soff = p - (char *)fdt;
>  
> -	if (((p + oldlen) < p) || ((p + oldlen) > end))
> +	if ((oldlen < 0) || (soff + oldlen < soff) || (soff + oldlen > dsize))
>  		return -FDT_ERR_BADOFFSET;
> -	if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
> +	if ((p < (char *)fdt) || (dsize + newlen < oldlen))
>  		return -FDT_ERR_BADOFFSET;
> -	if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
> +	if (dsize - oldlen + newlen > fdt_totalsize(fdt))
>  		return -FDT_ERR_NOSPACE;
> -	memmove(p + newlen, p + oldlen, end - p - oldlen);
> +	memmove(p + newlen, p + oldlen, ((char *)fdt + dsize) - (p + oldlen));
>  	return 0;
>  }
>  
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libfdt: fix undefined behaviour in fdt_splice_()

[Jan Beulich]

David,

On 11.03.2020 05:59, David Gibson wrote:
> On Thu, Mar 05, 2020 at 03:04:45PM +0100, Jan Beulich wrote:
>> libfdt: fix undefined behaviour in fdt_splice_()
>>
>> Along the lines of commit d0b3ab0a0f46 ("libfdt: Fix undefined behaviour
>> in fdt_offset_ptr()"), fdt_splice_() similarly may not use pointer
>> arithmetic to do overflow checks. (The left side of the checks added by
>> d4c7c25c9ed1 ["libfdt: check for potential overrun in _fdt_splice()"]
>> doesn't really lend itself to similar replacement though.)
>>
>> Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> 
> Applied, thanks.

would you mind pointing me at the tree that you applied this to?
From looking around I got the impression that

https://git.kernel.org/pub/scm/utils/dtc/dtc.git

would be the master tree, but I can't see any commit there newer
than from 8 days ago. The reason I'm asking is because I found
the issue originally in the Xen tree, which had imported libfdt
from the DTC project, and in order to submit the (backported)
fix there I'd like to be able to name the master tree's commit
hash.

Thanks, Jan

Re: [PATCH] libfdt: fix undefined behaviour in fdt_splice_()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]

On Thu, Mar 12, 2020 at 09:19:06AM +0100, Jan Beulich wrote:
> David,
> 
> On 11.03.2020 05:59, David Gibson wrote:
> > On Thu, Mar 05, 2020 at 03:04:45PM +0100, Jan Beulich wrote:
> >> libfdt: fix undefined behaviour in fdt_splice_()
> >>
> >> Along the lines of commit d0b3ab0a0f46 ("libfdt: Fix undefined behaviour
> >> in fdt_offset_ptr()"), fdt_splice_() similarly may not use pointer
> >> arithmetic to do overflow checks. (The left side of the checks added by
> >> d4c7c25c9ed1 ["libfdt: check for potential overrun in _fdt_splice()"]
> >> doesn't really lend itself to similar replacement though.)
> >>
> >> Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> > 
> > Applied, thanks.
> 
> would you mind pointing me at the tree that you applied this to?
> >From looking around I got the impression that
> 
> https://git.kernel.org/pub/scm/utils/dtc/dtc.git
> 
> would be the master tree, but I can't see any commit there newer
> than from 8 days ago. The reason I'm asking is because I found
> the issue originally in the Xen tree, which had imported libfdt
> from the DTC project, and in order to submit the (backported)
> fix there I'd like to be able to name the master tree's commit
> hash.

Oh, sorry, I just forgot to push it out from my local tree.  It should
be there now.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]