Re: [PATCH] x86/insn: Use get_unaligned() instead of memcpy()

From: Masami Hiramatsu <mhiramat@kernel.org>
To: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	the arch/x86 maintainers <x86@kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Ian Rogers <irogers@google.com>
Subject: Re: [PATCH] x86/insn: Use get_unaligned() instead of memcpy()
Date: Thu, 30 Sep 2021 09:00:54 +0900	[thread overview]
Message-ID: <20210930090054.dc54a2ce50dda8f196084c75@kernel.org> (raw)
In-Reply-To: <YVSsIkj9Z29TyUjE@zn.tnic>

On Wed, 29 Sep 2021 20:10:42 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Use get_unaligned() instead of memcpy() to access potentially unaligned
> memory, which, when accessed through a pointer, leads to undefined
> behavior. get_unaligned() describes much better what is happening there
> anyway even if memcpy() does the job.
> 
> No functional changes.

Thank you very much to fix this! This looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

> 
> Fixes: 5ba1071f7554 ("x86/insn, tools/x86: Fix undefined behavior due to potential unaligned accesses")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/lib/insn.c                   |  5 +++--
>  tools/arch/x86/lib/insn.c             |  5 +++--
>  tools/include/asm-generic/unaligned.h | 23 +++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 4 deletions(-)
>  create mode 100644 tools/include/asm-generic/unaligned.h
> 
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index c565def611e2..55e371cc69fd 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -13,6 +13,7 @@
>  #endif
>  #include <asm/inat.h> /*__ignore_sync_check__ */
>  #include <asm/insn.h> /* __ignore_sync_check__ */
> +#include <asm/unaligned.h> /* __ignore_sync_check__ */
>  
>  #include <linux/errno.h>
>  #include <linux/kconfig.h>
> @@ -37,10 +38,10 @@
>  	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>  
>  #define __get_next(t, insn)	\
> -	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
> +	({ t r = get_unaligned((t *)(insn)->next_byte); (insn)->next_byte += sizeof(t); leXX_to_cpu(t, r); })
>  
>  #define __peek_nbyte_next(t, insn, n)	\
> -	({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
> +	({ t r = get_unaligned((t *)(insn)->next_byte + n); 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 797699462cd8..8fd63a067308 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -13,6 +13,7 @@
>  #endif
>  #include "../include/asm/inat.h" /* __ignore_sync_check__ */
>  #include "../include/asm/insn.h" /* __ignore_sync_check__ */
> +#include "../include/asm-generic/unaligned.h" /* __ignore_sync_check__ */
>  
>  #include <linux/errno.h>
>  #include <linux/kconfig.h>
> @@ -37,10 +38,10 @@
>  	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>  
>  #define __get_next(t, insn)	\
> -	({ t r; memcpy(&r, insn->next_byte, sizeof(t)); insn->next_byte += sizeof(t); leXX_to_cpu(t, r); })
> +	({ t r = get_unaligned((t *)(insn)->next_byte); (insn)->next_byte += sizeof(t); leXX_to_cpu(t, r); })
>  
>  #define __peek_nbyte_next(t, insn, n)	\
> -	({ t r; memcpy(&r, (insn)->next_byte + n, sizeof(t)); leXX_to_cpu(t, r); })
> +	({ t r = get_unaligned((t *)(insn)->next_byte + n); 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/include/asm-generic/unaligned.h b/tools/include/asm-generic/unaligned.h
> new file mode 100644
> index 000000000000..47387c607035
> --- /dev/null
> +++ b/tools/include/asm-generic/unaligned.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copied from the kernel sources to tools/perf/:
> + */
> +
> +#ifndef __TOOLS_LINUX_ASM_GENERIC_UNALIGNED_H
> +#define __TOOLS_LINUX_ASM_GENERIC_UNALIGNED_H
> +
> +#define __get_unaligned_t(type, ptr) ({						\
> +	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
> +	__pptr->x;								\
> +})
> +
> +#define __put_unaligned_t(type, val, ptr) do {					\
> +	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\
> +	__pptr->x = (val);							\
> +} while (0)
> +
> +#define get_unaligned(ptr)	__get_unaligned_t(typeof(*(ptr)), (ptr))
> +#define put_unaligned(val, ptr) __put_unaligned_t(typeof(*(ptr)), (val), (ptr))
> +
> +#endif /* __TOOLS_LINUX_ASM_GENERIC_UNALIGNED_H */
> +
> -- 
> 2.29.2
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

-- 
Masami Hiramatsu <mhiramat@kernel.org>