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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 081B0C433F5 for ; Sat, 16 Oct 2021 06:42:14 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A66C661184 for ; Sat, 16 Oct 2021 06:42:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A66C661184 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=csgroup.eu Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 316C26B0072; Sat, 16 Oct 2021 02:42:13 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2C6B36B0073; Sat, 16 Oct 2021 02:42:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 18EA56B0074; Sat, 16 Oct 2021 02:42:13 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0229.hostedemail.com [216.40.44.229]) by kanga.kvack.org (Postfix) with ESMTP id 0BD3B6B0072 for ; Sat, 16 Oct 2021 02:42:13 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 44BDC1823885F for ; Sat, 16 Oct 2021 06:42:12 +0000 (UTC) X-FDA: 78701356104.21.C928FFC Received: from pegase2.c-s.fr (pegase2.c-s.fr [93.17.235.10]) by imf24.hostedemail.com (Postfix) with ESMTP id 537F8B00009F for ; Sat, 16 Oct 2021 06:42:10 +0000 (UTC) Received: from localhost (mailhub3.si.c-s.fr [172.26.127.67]) by localhost (Postfix) with ESMTP id 4HWYTZ2jPNz9sSL; Sat, 16 Oct 2021 08:42:10 +0200 (CEST) X-Virus-Scanned: amavisd-new at c-s.fr Received: from pegase2.c-s.fr ([172.26.127.65]) by localhost (pegase2.c-s.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id K3NXrTGrnHQ3; Sat, 16 Oct 2021 08:42:10 +0200 (CEST) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase2.c-s.fr (Postfix) with ESMTP id 4HWYTZ1gXQz9sSH; Sat, 16 Oct 2021 08:42:10 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id 1E83F8B765; Sat, 16 Oct 2021 08:42:10 +0200 (CEST) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id 09O0ACPW_BH4; Sat, 16 Oct 2021 08:42:10 +0200 (CEST) Received: from PO20335.IDSI0.si.c-s.fr (unknown [192.168.203.36]) by messagerie.si.c-s.fr (Postfix) with ESMTP id D7EA88B763; Sat, 16 Oct 2021 08:42:08 +0200 (CEST) Subject: Re: [PATCH v2 12/13] lkdtm: Fix execute_[user]_location() To: Kees Cook Cc: Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Andrew Morton , "James E.J. Bottomley" , Helge Deller , Arnd Bergmann , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-ia64@vger.kernel.org, linux-parisc@vger.kernel.org, linux-arch@vger.kernel.org, linux-mm@kvack.org References: <202110151428.187B1CF@keescook> From: Christophe Leroy Message-ID: <9b4c39d4-1322-89af-585c-679a574576a2@csgroup.eu> Date: Sat, 16 Oct 2021 08:42:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <202110151428.187B1CF@keescook> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: fr-FR X-Rspamd-Queue-Id: 537F8B00009F Authentication-Results: imf24.hostedemail.com; dkim=none; spf=pass (imf24.hostedemail.com: domain of christophe.leroy@csgroup.eu designates 93.17.235.10 as permitted sender) smtp.mailfrom=christophe.leroy@csgroup.eu; dmarc=none X-Stat-Signature: bj9yeny7fha8ikq5csa79y4ci8y9zoxb X-Rspamd-Server: rspam05 X-HE-Tag: 1634366530-739944 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Le 15/10/2021 =C3=A0 23:31, Kees Cook a =C3=A9crit=C2=A0: > On Thu, Oct 14, 2021 at 07:50:01AM +0200, Christophe Leroy wrote: >> execute_location() and execute_user_location() intent >> to copy do_nothing() text and execute it at a new location. >> However, at the time being it doesn't copy do_nothing() function >> but do_nothing() function descriptor which still points to the >> original text. So at the end it still executes do_nothing() at >> its original location allthough using a copied function descriptor. >> >> So, fix that by really copying do_nothing() text and build a new >> function descriptor by copying do_nothing() function descriptor and >> updating the target address with the new location. >> >> Also fix the displayed addresses by dereferencing do_nothing() >> function descriptor. >> >> Signed-off-by: Christophe Leroy >> --- >> drivers/misc/lkdtm/perms.c | 25 +++++++++++++++++++++---- >> include/asm-generic/sections.h | 5 +++++ >> 2 files changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index 5266dc28df6e..96b3ebfcb8ed 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -44,19 +44,32 @@ static noinline void do_overwritten(void) >> return; >> } >> =20 >> +static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) >> +{ >> + memcpy(fdesc, do_nothing, sizeof(*fdesc)); >> + fdesc->addr =3D (unsigned long)dst; >> + barrier(); >> + >> + return fdesc; >> +} >=20 > How about collapsing the "have_function_descriptors()" check into > setup_function_descriptor()? >=20 > static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) > { > if (__is_defined(HAVE_FUNCTION_DESCRIPTORS)) { > memcpy(fdesc, do_nothing, sizeof(*fdesc)); > fdesc->addr =3D (unsigned long)dst; > barrier(); > return fdesc; > } else { > return dst; > } > } Ok >=20 >> + >> static noinline void execute_location(void *dst, bool write) >> { >> void (*func)(void) =3D dst; >> + func_desc_t fdesc; >> + void *do_nothing_text =3D dereference_function_descriptor(do_nothing= ); >> =20 >> - pr_info("attempting ok execution at %px\n", do_nothing); >> + pr_info("attempting ok execution at %px\n", do_nothing_text); >> do_nothing(); >> =20 >> if (write =3D=3D CODE_WRITE) { >> - memcpy(dst, do_nothing, EXEC_SIZE); >> + memcpy(dst, do_nothing_text, EXEC_SIZE); >> flush_icache_range((unsigned long)dst, >> (unsigned long)dst + EXEC_SIZE); >> } >> pr_info("attempting bad execution at %px\n", func); >> + if (have_function_descriptors()) >> + func =3D setup_function_descriptor(&fdesc, dst); >> func(); >> pr_err("FAIL: func returned\n"); >> } >> @@ -67,15 +80,19 @@ static void execute_user_location(void *dst) >> =20 >> /* Intentionally crossing kernel/user memory boundary. */ >> void (*func)(void) =3D dst; >> + func_desc_t fdesc; >> + void *do_nothing_text =3D dereference_function_descriptor(do_nothing= ); >> =20 >> - pr_info("attempting ok execution at %px\n", do_nothing); >> + pr_info("attempting ok execution at %px\n", do_nothing_text); >> do_nothing(); >> =20 >> - copied =3D access_process_vm(current, (unsigned long)dst, do_nothing= , >> + copied =3D access_process_vm(current, (unsigned long)dst, do_nothing= _text, >> EXEC_SIZE, FOLL_WRITE); >> if (copied < EXEC_SIZE) >> return; >> pr_info("attempting bad execution at %px\n", func); >> + if (have_function_descriptors()) >> + func =3D setup_function_descriptor(&fdesc, dst); >> func(); >> pr_err("FAIL: func returned\n"); >> } >=20 >=20 >> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sect= ions.h >> index 76163883c6ff..d225318538bd 100644 >> --- a/include/asm-generic/sections.h >> +++ b/include/asm-generic/sections.h >> @@ -70,6 +70,11 @@ typedef struct { >> } func_desc_t; >> #endif >> =20 >> +static inline bool have_function_descriptors(void) >> +{ >> + return __is_defined(HAVE_FUNCTION_DESCRIPTORS); >> +} >> + >> /* random extra sections (if any). Override >> * in asm/sections.h */ >> #ifndef arch_is_kernel_text >=20 > This hunk seems like it should live in a separate patch. >=20 Ok I move it in a previous patch.