libfdt: eliminate UB pointer validation

[Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation

Posted by Jan Beulich 4 years, 1 month ago

The other day, in the context of what is now cf38b4926e2b ("xmalloc:
guard against integer overflow"), Andrew had suggested to look into
using gcc's __builtin_*_overflow(). The functions don't lend themselves
to be used there with the logic currently in place (albeit we may still
want to consider adjustments there), but I then went on to see whether
we have any other overflow checks wanting conversion. One thing I
noticed was that for unsigned integer arithmetic the compiler normally
does fine recognizing the intent without using the builtins. And while
I didn't to spot any signed integer overflow checks (which likely
would have been UB anyway), I did spot two in libfdt. After figuring
out where exactly that code was taken from, I spotted a fix for one of
the two in the upstream repo, and I submitted a fix for the other one
there first. Here are the backports thereof, as I don't myself want to
get into the business of bumping the libfdt version in our repo.

1: Fix undefined behaviour in fdt_offset_ptr()
2: fix undefined behaviour in _fdt_splice()

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation

Posted by Julien Grall 4 years, 1 month ago

Hi Jan,

On 13/03/2020 07:32, Jan Beulich wrote:
> The other day, in the context of what is now cf38b4926e2b ("xmalloc:
> guard against integer overflow"), Andrew had suggested to look into
> using gcc's __builtin_*_overflow(). The functions don't lend themselves
> to be used there with the logic currently in place (albeit we may still
> want to consider adjustments there), but I then went on to see whether
> we have any other overflow checks wanting conversion. One thing I
> noticed was that for unsigned integer arithmetic the compiler normally
> does fine recognizing the intent without using the builtins. And while
> I didn't to spot any signed integer overflow checks (which likely
> would have been UB anyway), I did spot two in libfdt. After figuring
> out where exactly that code was taken from, I spotted a fix for one of
> the two in the upstream repo, and I submitted a fix for the other one
> there first. Here are the backports thereof, as I don't myself want to
> get into the business of bumping the libfdt version in our repo.

Our version of libfdt is now 7 years old. We should probably look at 
upgrading to the latest one.

I will add it in my TODO list.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr()

Posted by Jan Beulich 4 years, 1 month ago

From: David Gibson <david@gibson.dropbear.id.au>

Using pointer arithmetic to generate a pointer outside a known object is,
technically, undefined behaviour in C.  Unfortunately, we were using that
in fdt_offset_ptr() to detect overflows.

To fix this we need to do our bounds / overflow checking on the offsets
before constructing pointers from them.

Reported-by: David Binderman <dcb314@hotmail.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[upstream commit d0b3ab0a0f46ac929b4713da46f7fdcd893dd3bd]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libfdt/fdt.c
+++ b/xen/common/libfdt/fdt.c
@@ -74,18 +74,19 @@ int fdt_check_header(const void *fdt)
 
 const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 {
-	const char *p;
+	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
+
+	if ((absoffset < offset)
+	    || ((absoffset + len) < absoffset)
+	    || (absoffset + len) > fdt_totalsize(fdt))
+		return NULL;
 
 	if (fdt_version(fdt) >= 0x11)
 		if (((offset + len) < offset)
 		    || ((offset + len) > fdt_size_dt_struct(fdt)))
 			return NULL;
 
-	p = _fdt_offset_ptr(fdt, offset);
-
-	if (p + len < p)
-		return NULL;
-	return p;
+	return _fdt_offset_ptr(fdt, offset);
 }
 
 uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr()

Posted by Julien Grall 4 years, 1 month ago

Hi Jan,

On 13/03/2020 07:35, Jan Beulich wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Using pointer arithmetic to generate a pointer outside a known object is,
> technically, undefined behaviour in C.  Unfortunately, we were using that
> in fdt_offset_ptr() to detect overflows.
> 
> To fix this we need to do our bounds / overflow checking on the offsets
> before constructing pointers from them.
> 
> Reported-by: David Binderman <dcb314@hotmail.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [upstream commit d0b3ab0a0f46ac929b4713da46f7fdcd893dd3bd]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> 
> --- a/xen/common/libfdt/fdt.c
> +++ b/xen/common/libfdt/fdt.c
> @@ -74,18 +74,19 @@ int fdt_check_header(const void *fdt)
>   
>   const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>   {
> -	const char *p;
> +	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> +
> +	if ((absoffset < offset)
> +	    || ((absoffset + len) < absoffset)
> +	    || (absoffset + len) > fdt_totalsize(fdt))
> +		return NULL;
>   
>   	if (fdt_version(fdt) >= 0x11)
>   		if (((offset + len) < offset)
>   		    || ((offset + len) > fdt_size_dt_struct(fdt)))
>   			return NULL;
>   
> -	p = _fdt_offset_ptr(fdt, offset);
> -
> -	if (p + len < p)
> -		return NULL;
> -	return p;
> +	return _fdt_offset_ptr(fdt, offset);
>   }
>   
>   uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice()

Posted by Jan Beulich 4 years, 1 month ago

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.

[upstream commit 73d6e9ecb4179b510408bc526240f829262df361]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libfdt/fdt_rw.c
+++ b/xen/common/libfdt/fdt_rw.c
@@ -87,7 +87,7 @@ static int _fdt_rw_check_header(void *fd
 			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);
 }
@@ -95,13 +95,14 @@ 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 ((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;
 }
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice()

Posted by Julien Grall 4 years, 1 month ago

Hi Jan,

On 13/03/2020 07:35, Jan Beulich wrote:
> 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.
> 
> [upstream commit 73d6e9ecb4179b510408bc526240f829262df361]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> 
> --- a/xen/common/libfdt/fdt_rw.c
> +++ b/xen/common/libfdt/fdt_rw.c
> @@ -87,7 +87,7 @@ static int _fdt_rw_check_header(void *fd
>   			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);
>   }
> @@ -95,13 +95,14 @@ 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 ((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;
>   }
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel