From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-21.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 10501C433FE for ; Wed, 9 Dec 2020 18:05:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BABC722ADC for ; Wed, 9 Dec 2020 18:05:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732477AbgLISFw (ORCPT ); Wed, 9 Dec 2020 13:05:52 -0500 Received: from mail.kernel.org ([198.145.29.99]:54440 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732461AbgLISFw (ORCPT ); Wed, 9 Dec 2020 13:05:52 -0500 Date: Wed, 9 Dec 2020 15:05:19 -0300 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1607537111; bh=w7bipq0SUrgQUd+zYlUutmyVUYkj9IYNtEEIakkju00=; h=From:To:Cc:Subject:References:In-Reply-To:From; b=TzQPzdOUOxcyIc1xXcd8aFfbBjmgjwHcEFVSmNHZ4/SgH1MOhpGr5erjq2K+yDltT v/FT2W447p4ywB9IA8WYpmuRQE4VGGqDsBpKE3GBlmMWzgzQ+S4Z7AL6XUmW6whPfG ydTtw+qrBQj9k/vckLJa7wZv0DVDvdr0/T3Oi0xlzM7a8FCOwl1jNgFhVB9KLIOEWo m2uk3AeSD+VOCNm6ixR/JnKKDSsUJe357tpORZQvfLrdTterugYcV0IZm4vC/QPDwH LZ2pGkAExPuibwB0uNOVBTz2K+dRh/wq1aiRDErNYqazlL8EEib0/k1AUOsgRsIqgt Uf3r7oLbEDV+g== From: Arnaldo Carvalho de Melo To: Masami Hiramatsu Cc: linux-kernel@vger.kernel.org, tip-bot2 for Masami Hiramatsu , linux-tip-commits@vger.kernel.org, syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com, Borislav Petkov , Srikar Dronamraju , stable@vger.kernel.org, x86@kernel.org Subject: Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes Message-ID: <20201209180519.GE185686@kernel.org> References: <160697103739.3146288.7437620795200799020.stgit@devnote2> <160709424307.3364.5849503551045240938.tip-bot2@tip-bot2> <20201205091256.14161a2e1606c527131efc06@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201205091256.14161a2e1606c527131efc06@kernel.org> X-Url: http://acmel.wordpress.com Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org Em Sat, Dec 05, 2020 at 09:12:56AM +0900, Masami Hiramatsu escreveu: > On Fri, 04 Dec 2020 15:04:03 -0000 > "tip-bot2 for Masami Hiramatsu" wrote: > > > The following commit has been merged into the x86/urgent branch of tip: > > > > Commit-ID: 9dc23f960adb9ce410ef835b32a2398fdb09c828 > > Gitweb: https://git.kernel.org/tip/9dc23f960adb9ce410ef835b32a2398fdb09c828 > > Author: Masami Hiramatsu > > AuthorDate: Thu, 03 Dec 2020 13:50:37 +09:00 > > Committer: Borislav Petkov > > CommitterDate: Fri, 04 Dec 2020 14:32:57 +01:00 > > > > x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes > > > > Since insn.prefixes.nbytes can be bigger than the size of > > insn.prefixes.bytes[] when a prefix is repeated, the proper check must > > be > > > > insn.prefixes.bytes[i] != 0 and i < 4 > > > > instead of using insn.prefixes.nbytes. > > > > Introduce a for_each_insn_prefix() macro for this purpose. Debugged by > > Kees Cook . > > > > [ bp: Massage commit message, sync with the respective header in tools/ > > and drop "we". ] > > > > Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints") > > Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com > > Signed-off-by: Masami Hiramatsu > > Signed-off-by: Borislav Petkov > > Reviewed-by: Srikar Dronamraju > > Cc: stable@vger.kernel.org > > Link: https://lkml.kernel.org/r/160697103739.3146288.7437620795200799020.stgit@devnote2 > > --- > > arch/x86/include/asm/insn.h | 15 +++++++++++++++ > > arch/x86/kernel/uprobes.c | 10 ++++++---- > > tools/arch/x86/include/asm/insn.h | 17 ++++++++++++++++- > > 3 files changed, 37 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h > > index 5c1ae3e..a8c3d28 100644 > > --- a/arch/x86/include/asm/insn.h > > +++ b/arch/x86/include/asm/insn.h > > @@ -201,6 +201,21 @@ static inline int insn_offset_immediate(struct insn *insn) > > return insn_offset_displacement(insn) + insn->displacement.nbytes; > > } > > > > +/** > > + * for_each_insn_prefix() -- Iterate prefixes in the instruction > > + * @insn: Pointer to struct insn. > > + * @idx: Index storage. > > + * @prefix: Prefix byte. > > + * > > + * Iterate prefix bytes of given @insn. Each prefix byte is stored in @prefix > > + * and the index is stored in @idx (note that this @idx is just for a cursor, > > + * do not change it.) > > + * Since prefixes.nbytes can be bigger than 4 if some prefixes > > + * are repeated, it cannot be used for looping over the prefixes. > > + */ > > +#define for_each_insn_prefix(insn, idx, prefix) \ > > + for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (prefix = insn->prefixes.bytes[idx]) != 0; idx++) > > + > > #define POP_SS_OPCODE 0x1f > > #define MOV_SREG_OPCODE 0x8e > > > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > > index 3fdaa04..138bdb1 100644 > > --- a/arch/x86/kernel/uprobes.c > > +++ b/arch/x86/kernel/uprobes.c > > @@ -255,12 +255,13 @@ static volatile u32 good_2byte_insns[256 / 32] = { > > > > static bool is_prefix_bad(struct insn *insn) > > { > > + insn_byte_t p; > > int i; > > > > - for (i = 0; i < insn->prefixes.nbytes; i++) { > > + for_each_insn_prefix(insn, i, p) { > > insn_attr_t attr; > > > > - attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]); > > + attr = inat_get_opcode_attribute(p); > > switch (attr) { > > case INAT_MAKE_PREFIX(INAT_PFX_ES): > > case INAT_MAKE_PREFIX(INAT_PFX_CS): > > @@ -715,6 +716,7 @@ static const struct uprobe_xol_ops push_xol_ops = { > > static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > > { > > u8 opc1 = OPCODE1(insn); > > + insn_byte_t p; > > int i; > > > > switch (opc1) { > > @@ -746,8 +748,8 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > > * Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix. > > * No one uses these insns, reject any branch insns with such prefix. > > */ > > - for (i = 0; i < insn->prefixes.nbytes; i++) { > > - if (insn->prefixes.bytes[i] == 0x66) > > + for_each_insn_prefix(insn, i, p) { > > + if (p == 0x66) > > return -ENOTSUPP; > > } > > > > diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h > > index 568854b..a8c3d28 100644 > > --- a/tools/arch/x86/include/asm/insn.h > > +++ b/tools/arch/x86/include/asm/insn.h > > @@ -8,7 +8,7 @@ > > */ > > > > /* insn_attr_t is defined in inat.h */ > > -#include "inat.h" > > +#include > > This may break tools/objtool build. Please keep "inat.h". And also it would be interesting to avoid updating both the kernel and the tools/ copy, otherwise one would have to test the tools build, which may break with such updates. The whole point of the copy is to avoid that, otherwise we could just use the kernel files directly. - Arnaldo