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=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 E330EC433ED for ; Fri, 16 Apr 2021 13:52:48 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4EC0E61152 for ; Fri, 16 Apr 2021 13:52:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4EC0E61152 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=tempfail smtp.mailfrom=linux-audit-bounces@redhat.com Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-502-x7CfGYq9NzqMCu8sFn4atA-1; Fri, 16 Apr 2021 09:52:45 -0400 X-MC-Unique: x7CfGYq9NzqMCu8sFn4atA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 17EAF1006C83; Fri, 16 Apr 2021 13:52:42 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8479160C16; Fri, 16 Apr 2021 13:52:41 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id 4379B9968; Fri, 16 Apr 2021 13:52:39 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 13GDfCYg030916 for ; Fri, 16 Apr 2021 09:41:13 -0400 Received: by smtp.corp.redhat.com (Postfix) id B3CF220BDB14; Fri, 16 Apr 2021 13:41:12 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast03.extmail.prod.ext.rdu2.redhat.com [10.11.55.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AE4FB20BDB1C for ; Fri, 16 Apr 2021 13:41:09 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3F82680C8DB for ; Fri, 16 Apr 2021 13:41:09 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by relay.mimecast.com with ESMTP id us-mta-510-t-iWLz_-MDOezWFh1zVHMQ-1; Fri, 16 Apr 2021 09:41:03 -0400 X-MC-Unique: t-iWLz_-MDOezWFh1zVHMQ-1 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0F61F11B3; Fri, 16 Apr 2021 06:34:53 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.1.222]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0AD323F85F; Fri, 16 Apr 2021 06:34:50 -0700 (PDT) Date: Fri, 16 Apr 2021 14:34:41 +0100 From: Mark Rutland To: Catalin Marinas , will@kernel.org Subject: Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat Message-ID: <20210416133431.GA2303@C02TD0UTHF1T.local> References: <20210416075533.7720-1-zhe.he@windriver.com> <20210416123322.GA23184@arm.com> MIME-Version: 1.0 In-Reply-To: <20210416123322.GA23184@arm.com> X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-loop: linux-audit@redhat.com X-Mailman-Approved-At: Fri, 16 Apr 2021 09:51:54 -0400 Cc: He Zhe , oleg@redhat.com, linux-kernel@vger.kernel.org, linux-audit@redhat.com, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=linux-audit-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Fri, Apr 16, 2021 at 01:33:22PM +0100, Catalin Marinas wrote: > On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote: > > The general version of is_syscall_success does not handle 32-bit > > compatible case, which would cause 32-bit negative return code to be > > recoganized as a positive number later and seen as a "success". > > > > Since is_compat_thread is defined in compat.h, implementing > > is_syscall_success in ptrace.h would introduce build failure due to > > recursive inclusion of some basic headers like mutex.h. We put the > > implementation to ptrace.c > > > > Signed-off-by: He Zhe > > --- > > arch/arm64/include/asm/ptrace.h | 3 +++ > > arch/arm64/kernel/ptrace.c | 10 ++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > > index e58bca832dff..3c415e9e5d85 100644 > > --- a/arch/arm64/include/asm/ptrace.h > > +++ b/arch/arm64/include/asm/ptrace.h > > @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc) > > regs->regs[0] = rc; > > } > > > > +extern inline int is_syscall_success(struct pt_regs *regs); > > +#define is_syscall_success(regs) is_syscall_success(regs) > > + > > /** > > * regs_get_kernel_argument() - get Nth function argument in kernel > > * @regs: pt_regs of that context > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > > index 170f42fd6101..3266201f8c60 100644 > > --- a/arch/arm64/kernel/ptrace.c > > +++ b/arch/arm64/kernel/ptrace.c > > @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task) > > else > > return valid_native_regs(regs); > > } > > + > > +inline int is_syscall_success(struct pt_regs *regs) > > +{ > > + unsigned long val = regs->regs[0]; > > + > > + if (is_compat_thread(task_thread_info(current))) > > + val = sign_extend64(val, 31); > > + > > + return !IS_ERR_VALUE(val); > > +} > > It's better to use compat_user_mode(regs) here instead of > is_compat_thread(). It saves us from worrying whether regs are for the > current context. > > I think we should change regs_return_value() instead. This function > seems to be called from several other places and it has the same > potential problems if called on compat pt_regs. I think this is a problem we created for ourselves back in commit: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return) AFAICT, the perf regs samples are the only place this matters, since for ptrace the compat regs are implicitly truncated to compat_ulong_t, and audit expects the non-truncated return value. Other architectures don't truncate here, so I think we're setting ourselves up for a game of whack-a-mole to truncate and extend wherever we need to. Given that, I suspect it'd be better to do something like the below. Will, thoughts? Mark. ---->8---- >>From df0f7c160240d9ee6f20f87a180326d3253e80fb Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Fri, 16 Apr 2021 13:58:54 +0100 Subject: [PATCH] arm64: perf: truncate compat regs For compat userspace, it doesn't generally make sense for the upper 32 bits of GPRs to be set, as these bits don't really exist in AArch32. However, for structural reasons the kernel may transiently set the upper 32 bits of registers in pt_regs at points where a perf sample can be taken. We tried to avoid this happening in commit: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return") ... by having invoke_syscall() truncate the return value for compat tasks, with helpers in extending the return value when required. Unfortunately this is not complete, as there are other places where we assign the return value, such as when el0_svc_common() sets up a return of -ENOSYS. Further, this approach breaks the audit code, which relies on the upper 32 bits of the return value. Instead, let's have the perf code explicitly truncate the user regs to 32 bits, and otherwise preserve those within the kernel. Fixes: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return") Signed-off-by: Mark Rutland Cc: Will Deacon --- arch/arm64/include/asm/syscall.h | 11 +---------- arch/arm64/kernel/perf_regs.c | 26 ++++++++++++++++---------- arch/arm64/kernel/syscall.c | 3 --- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h index cfc0672013f6..0ebeaf6dbd45 100644 --- a/arch/arm64/include/asm/syscall.h +++ b/arch/arm64/include/asm/syscall.h @@ -35,9 +35,6 @@ static inline long syscall_get_error(struct task_struct *task, { unsigned long error = regs->regs[0]; - if (is_compat_thread(task_thread_info(task))) - error = sign_extend64(error, 31); - return IS_ERR_VALUE(error) ? error : 0; } @@ -51,13 +48,7 @@ static inline void syscall_set_return_value(struct task_struct *task, struct pt_regs *regs, int error, long val) { - if (error) - val = error; - - if (is_compat_thread(task_thread_info(task))) - val = lower_32_bits(val); - - regs->regs[0] = val; + regs->regs[0] = (long) error ? error : val; } #define SYSCALL_MAX_ARGS 6 diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c index f6f58e6265df..296f0c55b4e2 100644 --- a/arch/arm64/kernel/perf_regs.c +++ b/arch/arm64/kernel/perf_regs.c @@ -9,6 +9,17 @@ #include #include +static u64 __perf_reg_value(struct pt_regs *regs, int idx) +{ + if ((u32)idx == PERF_REG_ARM64_SP) + return regs->sp; + + if ((u32)idx == PERF_REG_ARM64_PC) + return regs->pc; + + return regs->regs[idx]; +} + u64 perf_reg_value(struct pt_regs *regs, int idx) { if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX)) @@ -38,20 +49,15 @@ u64 perf_reg_value(struct pt_regs *regs, int idx) */ if (compat_user_mode(regs)) { if ((u32)idx == PERF_REG_ARM64_SP) - return regs->compat_sp; + return lower_32_bits(regs->compat_sp); if ((u32)idx == PERF_REG_ARM64_LR) - return regs->compat_lr; + return lower_32_bits(regs->compat_lr); if (idx == 15) - return regs->pc; + return lower_32_bits(regs->pc); + return lower_32_bits(__perf_reg_value(regs, idx)); } - if ((u32)idx == PERF_REG_ARM64_SP) - return regs->sp; - - if ((u32)idx == PERF_REG_ARM64_PC) - return regs->pc; - - return regs->regs[idx]; + return __perf_reg_value(regs, idx); } #define REG_RESERVED (~((1ULL << PERF_REG_ARM64_MAX) - 1)) diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c index b9cf12b271d7..84314fae4b5c 100644 --- a/arch/arm64/kernel/syscall.c +++ b/arch/arm64/kernel/syscall.c @@ -51,9 +51,6 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno, ret = do_ni_syscall(regs, scno); } - if (is_compat_task()) - ret = lower_32_bits(ret); - regs->regs[0] = ret; } -- 2.11.0 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit