* [PATCH 1/2] x86/insn: Fix some potential undefined behavior.
@ 2020-10-15 6:21 Ian Rogers
2020-10-15 6:21 ` [PATCH 2/2] tools/x86: " Ian Rogers
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Ian Rogers @ 2020-10-15 6:21 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Peter Zijlstra (Intel),
Masami Hiramatsu, Josh Poimboeuf, linux-kernel, Adrian Hunter,
Arnaldo Carvalho de Melo
Cc: Numfor Mbiziwo-Tiapo, Ian Rogers
From: Numfor Mbiziwo-Tiapo <nums@google.com>
If insn_init is given a NULL kaddr and 0 buflen then validate_next will
perform arithmetic on NULL, add a guard to avoid this.
Don't perform unaligned loads in __get_next and __peek_nbyte_next as
these are forms of undefined behavior.
These problems were identified using the undefined behavior sanitizer
(ubsan) with the tools version of the code and perf test. Part of this
patch was previously posted here:
https://lore.kernel.org/lkml/20190724184512.162887-4-nums@google.com/
Signed-off-by: Ian Rogers <irogers@google.com>
Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
arch/x86/lib/insn.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 404279563891..57236940de46 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -17,13 +17,13 @@
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
- ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
+ ((insn)->end_kaddr != 0 && (insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
#define __get_next(t, insn) \
- ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); r; })
#define __peek_nbyte_next(t, insn, n) \
- ({ t r = *(t*)((insn)->next_byte + n); r; })
+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
--
2.28.0.1011.ga647a8990f-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] tools/x86: Fix some potential undefined behavior
2020-10-15 6:21 [PATCH 1/2] x86/insn: Fix some potential undefined behavior Ian Rogers
@ 2020-10-15 6:21 ` Ian Rogers
2020-10-15 8:31 ` Masami Hiramatsu
2020-10-15 8:01 ` [PATCH 1/2] x86/insn: " Ingo Molnar
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2020-10-15 6:21 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Peter Zijlstra (Intel),
Masami Hiramatsu, Josh Poimboeuf, linux-kernel, Adrian Hunter,
Arnaldo Carvalho de Melo
Cc: Numfor Mbiziwo-Tiapo, Ian Rogers
From: Numfor Mbiziwo-Tiapo <nums@google.com>
If insn_init is given a NULL kaddr and 0 buflen then validate_next will
perform arithmetic on NULL, add a guard to avoid this.
Don't perform unaligned loads in __get_next and __peek_nbyte_next as
these are forms of undefined behavior.
These problems were identified using the undefined behavior sanitizer
(ubsan) with perf test. Part of this patch was previously posted here:
https://lore.kernel.org/lkml/20190724184512.162887-4-nums@google.com/
Signed-off-by: Ian Rogers <irogers@google.com>
Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
tools/arch/x86/lib/insn.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index 0151dfc6da61..e8874a8cac2c 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -17,13 +17,13 @@
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
- ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
+ ((insn)->end_kaddr != 0 && (insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
#define __get_next(t, insn) \
- ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); r; })
#define __peek_nbyte_next(t, insn, n) \
- ({ t r = *(t*)((insn)->next_byte + n); r; })
+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
--
2.28.0.1011.ga647a8990f-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] x86/insn: Fix some potential undefined behavior.
2020-10-15 6:21 [PATCH 1/2] x86/insn: Fix some potential undefined behavior Ian Rogers
2020-10-15 6:21 ` [PATCH 2/2] tools/x86: " Ian Rogers
@ 2020-10-15 8:01 ` Ingo Molnar
2020-10-15 8:29 ` Masami Hiramatsu
2020-10-15 10:05 ` Peter Zijlstra
3 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2020-10-15 8:01 UTC (permalink / raw)
To: Ian Rogers
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Peter Zijlstra (Intel),
Masami Hiramatsu, Josh Poimboeuf, linux-kernel, Adrian Hunter,
Arnaldo Carvalho de Melo, Numfor Mbiziwo-Tiapo
* Ian Rogers <irogers@google.com> wrote:
> From: Numfor Mbiziwo-Tiapo <nums@google.com>
>
> If insn_init is given a NULL kaddr and 0 buflen then validate_next will
> perform arithmetic on NULL, add a guard to avoid this.
>
> Don't perform unaligned loads in __get_next and __peek_nbyte_next as
> these are forms of undefined behavior.
So, 'insn' is a kernel structure, usually allocated on the kernel stack.
How could these fields ever be unaligned?
>
> These problems were identified using the undefined behavior sanitizer
> (ubsan) with the tools version of the code and perf test. Part of this
> patch was previously posted here:
> https://lore.kernel.org/lkml/20190724184512.162887-4-nums@google.com/
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
> arch/x86/lib/insn.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 404279563891..57236940de46 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -17,13 +17,13 @@
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> - ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> + ((insn)->end_kaddr != 0 && (insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>
> #define __get_next(t, insn) \
> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); r; })
>
> #define __peek_nbyte_next(t, insn, n) \
> - ({ t r = *(t*)((insn)->next_byte + n); r; })
> + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
Is there any code generation side effect of this change to the resulting
code?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] x86/insn: Fix some potential undefined behavior.
2020-10-15 6:21 [PATCH 1/2] x86/insn: Fix some potential undefined behavior Ian Rogers
2020-10-15 6:21 ` [PATCH 2/2] tools/x86: " Ian Rogers
2020-10-15 8:01 ` [PATCH 1/2] x86/insn: " Ingo Molnar
@ 2020-10-15 8:29 ` Masami Hiramatsu
2020-10-15 10:05 ` Peter Zijlstra
3 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2020-10-15 8:29 UTC (permalink / raw)
To: Ian Rogers
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Peter Zijlstra (Intel),
Josh Poimboeuf, linux-kernel, Adrian Hunter,
Arnaldo Carvalho de Melo, Numfor Mbiziwo-Tiapo
On Wed, 14 Oct 2020 23:21:47 -0700
Ian Rogers <irogers@google.com> wrote:
> From: Numfor Mbiziwo-Tiapo <nums@google.com>
>
> If insn_init is given a NULL kaddr and 0 buflen then validate_next will
> perform arithmetic on NULL, add a guard to avoid this.
Maybe we should check the kaddr and end_kaddr existence in insn_init().
At least end_kaddr != 0 can be checked in insn_init() because it will
not be updated, right?
Actually, I expected that the caller checked it, but if you concern
the case, insn_init() must return -EINVAL for such case.
> Don't perform unaligned loads in __get_next and __peek_nbyte_next as
> these are forms of undefined behavior.
Hm, I thought x86 could handle such unaligned access. Of course, recent
change made the insn.c complied on another arch, so maybe it should
be treated.
BTW (and OT), would we have to take care all of those unaligned
access? ( e.g. int *p = somewhere; while (*p++) ... ?)
Also, you have to update the copy of this file under tools/ too.
Thank you,
>
> These problems were identified using the undefined behavior sanitizer
> (ubsan) with the tools version of the code and perf test. Part of this
> patch was previously posted here:
> https://lore.kernel.org/lkml/20190724184512.162887-4-nums@google.com/
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
> arch/x86/lib/insn.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 404279563891..57236940de46 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -17,13 +17,13 @@
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> - ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> + ((insn)->end_kaddr != 0 && (insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>
> #define __get_next(t, insn) \
> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); r; })
>
> #define __peek_nbyte_next(t, insn, n) \
> - ({ t r = *(t*)((insn)->next_byte + n); r; })
> + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> --
> 2.28.0.1011.ga647a8990f-goog
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tools/x86: Fix some potential undefined behavior
2020-10-15 6:21 ` [PATCH 2/2] tools/x86: " Ian Rogers
@ 2020-10-15 8:31 ` Masami Hiramatsu
0 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2020-10-15 8:31 UTC (permalink / raw)
To: Ian Rogers
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Peter Zijlstra (Intel),
Josh Poimboeuf, linux-kernel, Adrian Hunter,
Arnaldo Carvalho de Melo, Numfor Mbiziwo-Tiapo
Hi,
Please merge the change on arch/x86/lib/insn.c and tools/arch/x86/lib/insn.c
to a single patch for bisecting.
Thank you,
On Wed, 14 Oct 2020 23:21:48 -0700
Ian Rogers <irogers@google.com> wrote:
> From: Numfor Mbiziwo-Tiapo <nums@google.com>
>
> If insn_init is given a NULL kaddr and 0 buflen then validate_next will
> perform arithmetic on NULL, add a guard to avoid this.
>
> Don't perform unaligned loads in __get_next and __peek_nbyte_next as
> these are forms of undefined behavior.
>
> These problems were identified using the undefined behavior sanitizer
> (ubsan) with perf test. Part of this patch was previously posted here:
> https://lore.kernel.org/lkml/20190724184512.162887-4-nums@google.com/
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
> tools/arch/x86/lib/insn.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index 0151dfc6da61..e8874a8cac2c 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -17,13 +17,13 @@
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> - ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> + ((insn)->end_kaddr != 0 && (insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>
> #define __get_next(t, insn) \
> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); r; })
>
> #define __peek_nbyte_next(t, insn, n) \
> - ({ t r = *(t*)((insn)->next_byte + n); r; })
> + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); r; })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> --
> 2.28.0.1011.ga647a8990f-goog
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] x86/insn: Fix some potential undefined behavior.
2020-10-15 6:21 [PATCH 1/2] x86/insn: Fix some potential undefined behavior Ian Rogers
` (2 preceding siblings ...)
2020-10-15 8:29 ` Masami Hiramatsu
@ 2020-10-15 10:05 ` Peter Zijlstra
3 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2020-10-15 10:05 UTC (permalink / raw)
To: Ian Rogers
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Masami Hiramatsu, Josh Poimboeuf, linux-kernel,
Adrian Hunter, Arnaldo Carvalho de Melo, Numfor Mbiziwo-Tiapo
On Wed, Oct 14, 2020 at 11:21:47PM -0700, Ian Rogers wrote:
> From: Numfor Mbiziwo-Tiapo <nums@google.com>
>
> If insn_init is given a NULL kaddr and 0 buflen then validate_next will
> perform arithmetic on NULL, add a guard to avoid this.
How is this a problem? NULL is (void *)0, you can do arithmetic on that
just fine.
Is UBSAN taking drugs again?
> Don't perform unaligned loads in __get_next and __peek_nbyte_next as
> these are forms of undefined behavior.
Fair enough; that could actually be a problem when we start to
cross-build this stuff. A RISC hosted version of the x86 decoder could
indeed trip this up.
But also, these are two changes in one patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-15 10:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 6:21 [PATCH 1/2] x86/insn: Fix some potential undefined behavior Ian Rogers
2020-10-15 6:21 ` [PATCH 2/2] tools/x86: " Ian Rogers
2020-10-15 8:31 ` Masami Hiramatsu
2020-10-15 8:01 ` [PATCH 1/2] x86/insn: " Ingo Molnar
2020-10-15 8:29 ` Masami Hiramatsu
2020-10-15 10:05 ` Peter Zijlstra
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).