linux-kernel - Re: [PATCH 1/2] x86/insn: Fix some potential undefined behavior.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20201015172914.f8b7efb7b8e8a2a51f38b291@kernel.org>
Date:   Thu, 15 Oct 2020 17:29:14 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        linux-kernel@...r.kernel.org,
        Adrian Hunter <adrian.hunter@...el.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Numfor Mbiziwo-Tiapo <nums@...gle.com>
Subject: Re: [PATCH 1/2] x86/insn: Fix some potential undefined behavior.

On Wed, 14 Oct 2020 23:21:47 -0700
Ian Rogers <irogers@...gle.com> wrote:

> From: Numfor Mbiziwo-Tiapo <nums@...gle.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@...gle.com>
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@...gle.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@...nel.org>