* [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation
@ 2020-03-13 7:32 Jan Beulich
2020-03-13 7:35 ` [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr() Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2020-03-13 7:32 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, Julien Grall
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr()
2020-03-13 7:32 [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Jan Beulich
@ 2020-03-13 7:35 ` Jan Beulich
2020-03-17 14:51 ` Julien Grall
2020-03-13 7:35 ` [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice() Jan Beulich
2020-03-17 14:58 ` [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Julien Grall
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-03-13 7:35 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, Julien Grall
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice()
2020-03-13 7:32 [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Jan Beulich
2020-03-13 7:35 ` [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr() Jan Beulich
@ 2020-03-13 7:35 ` Jan Beulich
2020-03-17 14:56 ` Julien Grall
2020-03-17 14:58 ` [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Julien Grall
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-03-13 7:35 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, Julien Grall
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr()
2020-03-13 7:35 ` [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr() Jan Beulich
@ 2020-03-17 14:51 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2020-03-17 14:51 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Stefano Stabellini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice()
2020-03-13 7:35 ` [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice() Jan Beulich
@ 2020-03-17 14:56 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2020-03-17 14:56 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Stefano Stabellini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation
2020-03-13 7:32 [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Jan Beulich
2020-03-13 7:35 ` [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr() Jan Beulich
2020-03-13 7:35 ` [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice() Jan Beulich
@ 2020-03-17 14:58 ` Julien Grall
2 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2020-03-17 14:58 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Stefano Stabellini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-17 14:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 7:32 [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Jan Beulich
2020-03-13 7:35 ` [Xen-devel] [PATCH 1/2] libfdt: Fix undefined behaviour in fdt_offset_ptr() Jan Beulich
2020-03-17 14:51 ` Julien Grall
2020-03-13 7:35 ` [Xen-devel] [PATCH 2/2] libfdt: fix undefined behaviour in _fdt_splice() Jan Beulich
2020-03-17 14:56 ` Julien Grall
2020-03-17 14:58 ` [Xen-devel] [PATCH 0/2] libfdt: eliminate UB pointer validation Julien Grall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).