From: Numfor Mbiziwo-Tiapo <[email protected]>
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/[email protected]/
v4. Fixes a typo.
v3. Is a rebase picking up a fix for big endian architectures.
v2. removes the validate_next check and merges the 2 changes into one as
requested by Masami Hiramatsu <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
---
arch/x86/lib/insn.c | 4 ++--
tools/arch/x86/lib/insn.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 058f19b20465..c565def611e2 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -37,10 +37,10 @@
((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); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
#define __peek_nbyte_next(t, insn, n) \
- ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index c41f95815480..797699462cd8 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -37,10 +37,10 @@
((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); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
#define __peek_nbyte_next(t, insn, n) \
- ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
--
2.33.0.464.g1972c5931b-goog
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 5ba1071f7554c4027bdbd712a146111de57918de
Gitweb: https://git.kernel.org/tip/5ba1071f7554c4027bdbd712a146111de57918de
Author: Numfor Mbiziwo-Tiapo <[email protected]>
AuthorDate: Thu, 23 Sep 2021 09:18:43 -07:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Fri, 24 Sep 2021 12:37:38 +02:00
x86/insn, tools/x86: Fix undefined behavior due to potential unaligned accesses
Don't perform unaligned loads in __get_next() and __peek_nbyte_next() as
these are forms of undefined behavior:
"A pointer to an object or incomplete type may be converted to a pointer
to a different object or incomplete type. If the resulting pointer
is not correctly aligned for the pointed-to type, the behavior is
undefined."
(from http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
These problems were identified using the undefined behavior sanitizer
(ubsan) with the tools version of the code and perf test.
[ bp: Massage commit message. ]
Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/lib/insn.c | 4 ++--
tools/arch/x86/lib/insn.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 058f19b..c565def 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -37,10 +37,10 @@
((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); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
#define __peek_nbyte_next(t, insn, n) \
- ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index c41f958..7976994 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -37,10 +37,10 @@
((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); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
#define __peek_nbyte_next(t, insn, n) \
- ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
Em Thu, Sep 23, 2021 at 09:18:43AM -0700, Ian Rogers escreveu:
> From: Numfor Mbiziwo-Tiapo <[email protected]>
>
> 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/[email protected]/
Masami, if you're ok, just process it including the tools/ bit.
- Arnaldo
> v4. Fixes a typo.
>
> v3. Is a rebase picking up a fix for big endian architectures.
>
> v2. removes the validate_next check and merges the 2 changes into one as
> requested by Masami Hiramatsu <[email protected]>
>
> Signed-off-by: Ian Rogers <[email protected]>
> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> Acked-by: Masami Hiramatsu <[email protected]>
> ---
> arch/x86/lib/insn.c | 4 ++--
> tools/arch/x86/lib/insn.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 058f19b20465..c565def611e2 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -37,10 +37,10 @@
> ((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); leXX_to_cpu(t, r); })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
>
> #define __peek_nbyte_next(t, insn, n) \
> - ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
> + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index c41f95815480..797699462cd8 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -37,10 +37,10 @@
> ((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); leXX_to_cpu(t, r); })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
>
> #define __peek_nbyte_next(t, insn, n) \
> - ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
> + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> --
> 2.33.0.464.g1972c5931b-goog
--
- Arnaldo
On Fri, 24 Sep 2021 16:02:33 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Thu, Sep 23, 2021 at 09:18:43AM -0700, Ian Rogers escreveu:
> > From: Numfor Mbiziwo-Tiapo <[email protected]>
> >
> > 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/[email protected]/
>
> Masami, if you're ok, just process it including the tools/ bit.
Hi Arnaldo,
This version updates the tools/ too, so I think this is OK.
(do I need re-Ack?)
Thank you,
>
> - Arnaldo
>
> > v4. Fixes a typo.
> >
> > v3. Is a rebase picking up a fix for big endian architectures.
> >
> > v2. removes the validate_next check and merges the 2 changes into one as
> > requested by Masami Hiramatsu <[email protected]>
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> > Acked-by: Masami Hiramatsu <[email protected]>
> > ---
> > arch/x86/lib/insn.c | 4 ++--
> > tools/arch/x86/lib/insn.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> > index 058f19b20465..c565def611e2 100644
> > --- a/arch/x86/lib/insn.c
> > +++ b/arch/x86/lib/insn.c
> > @@ -37,10 +37,10 @@
> > ((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); leXX_to_cpu(t, r); })
> > + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
> >
> > #define __peek_nbyte_next(t, insn, n) \
> > - ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
> > + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
> >
> > #define get_next(t, insn) \
> > ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> > diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> > index c41f95815480..797699462cd8 100644
> > --- a/tools/arch/x86/lib/insn.c
> > +++ b/tools/arch/x86/lib/insn.c
> > @@ -37,10 +37,10 @@
> > ((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); leXX_to_cpu(t, r); })
> > + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
> >
> > #define __peek_nbyte_next(t, insn, n) \
> > - ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
> > + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
> >
> > #define get_next(t, insn) \
> > ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> > --
> > 2.33.0.464.g1972c5931b-goog
>
> --
>
> - Arnaldo
--
Masami Hiramatsu <[email protected]>
Em Sat, Sep 25, 2021 at 01:39:44PM +0900, Masami Hiramatsu escreveu:
> On Fri, 24 Sep 2021 16:02:33 -0300
> Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> > Em Thu, Sep 23, 2021 at 09:18:43AM -0700, Ian Rogers escreveu:
> > > From: Numfor Mbiziwo-Tiapo <[email protected]>
> > >
> > > 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/[email protected]/
> >
> > Masami, if you're ok, just process it including the tools/ bit.
>
> Hi Arnaldo,
>
> This version updates the tools/ too, so I think this is OK.
> (do I need re-Ack?)
So you want me to process it?
- Arnaldo
> Thank you,
>
> >
> > - Arnaldo
> >
> > > v4. Fixes a typo.
> > >
> > > v3. Is a rebase picking up a fix for big endian architectures.
> > >
> > > v2. removes the validate_next check and merges the 2 changes into one as
> > > requested by Masami Hiramatsu <[email protected]>
> > >
> > > Signed-off-by: Ian Rogers <[email protected]>
> > > Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> > > Acked-by: Masami Hiramatsu <[email protected]>
> > > ---
> > > arch/x86/lib/insn.c | 4 ++--
> > > tools/arch/x86/lib/insn.c | 4 ++--
> > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> > > index 058f19b20465..c565def611e2 100644
> > > --- a/arch/x86/lib/insn.c
> > > +++ b/arch/x86/lib/insn.c
> > > @@ -37,10 +37,10 @@
> > > ((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); leXX_to_cpu(t, r); })
> > > + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
> > >
> > > #define __peek_nbyte_next(t, insn, n) \
> > > - ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
> > > + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
> > >
> > > #define get_next(t, insn) \
> > > ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> > > diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> > > index c41f95815480..797699462cd8 100644
> > > --- a/tools/arch/x86/lib/insn.c
> > > +++ b/tools/arch/x86/lib/insn.c
> > > @@ -37,10 +37,10 @@
> > > ((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); leXX_to_cpu(t, r); })
> > > + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
> > >
> > > #define __peek_nbyte_next(t, insn, n) \
> > > - ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
> > > + ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
> > >
> > > #define get_next(t, insn) \
> > > ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
> > > --
> > > 2.33.0.464.g1972c5931b-goog
> >
> > --
> >
> > - Arnaldo
>
>
> --
> Masami Hiramatsu <[email protected]>
--
- Arnaldo
On Sun, Sep 26, 2021 at 08:59:35AM -0300, Arnaldo Carvalho de Melo wrote:
> So you want me to process it?
https://git.kernel.org/tip/5ba1071f7554c4027bdbd712a146111de57918de
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
We have get_unaligned() for a reason.
On September 23, 2021 9:18:43 AM PDT, Ian Rogers <[email protected]> wrote:
>From: Numfor Mbiziwo-Tiapo <[email protected]>
>
>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/[email protected]/
>
>v4. Fixes a typo.
>
>v3. Is a rebase picking up a fix for big endian architectures.
>
>v2. removes the validate_next check and merges the 2 changes into one as
>requested by Masami Hiramatsu <[email protected]>
>
>Signed-off-by: Ian Rogers <[email protected]>
>Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
>Acked-by: Masami Hiramatsu <[email protected]>
>---
> arch/x86/lib/insn.c | 4 ++--
> tools/arch/x86/lib/insn.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
>index 058f19b20465..c565def611e2 100644
>--- a/arch/x86/lib/insn.c
>+++ b/arch/x86/lib/insn.c
>@@ -37,10 +37,10 @@
> ((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); leXX_to_cpu(t, r); })
>+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
>
> #define __peek_nbyte_next(t, insn, n) \
>- ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
>+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
>diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
>index c41f95815480..797699462cd8 100644
>--- a/tools/arch/x86/lib/insn.c
>+++ b/tools/arch/x86/lib/insn.c
>@@ -37,10 +37,10 @@
> ((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); leXX_to_cpu(t, r); })
>+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
>
> #define __peek_nbyte_next(t, insn, n) \
>- ({ t r = *(t*)((insn)->next_byte + n); leXX_to_cpu(t, r); })
>+ ({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
>
> #define get_next(t, insn) \
> ({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Em Sun, Sep 26, 2021 at 05:25:44PM +0200, Borislav Petkov escreveu:
> On Sun, Sep 26, 2021 at 08:59:35AM -0300, Arnaldo Carvalho de Melo wrote:
> > So you want me to process it?
>
> https://git.kernel.org/tip/5ba1071f7554c4027bdbd712a146111de57918de
Ok, processed already, case closed :-)
- Arnaldo