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=-5.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT 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 9FF3AC10F0E for ; Fri, 12 Apr 2019 11:55:21 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 70C032171F for ; Fri, 12 Apr 2019 11:55:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="tgPLZaho" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 70C032171F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=I/CL5nV69IQQ06WDUNhiXLypaXjAEsn+ZLIlfZaudO0=; b=tgPLZahonH56Mx jx7D731EJTbBiF62/1R5HU0vnNuFIEia0J1t7geUbssBZgb4lAjsGHYlLAhzR+cW9lQu+W2QXsXg8 Y2/3q2RTdPO/91S6RnxSxBBwsD/76EqB8KNwgXZos3LJL/KlTeGDGBlVP/YHbcqlvwrqJOz1Yvcf4 sRT91jbjLCGrwjEYSdY2cV9/qwq423YZAXH6f2b9vn+qBYYF/lahF2JSZb2KrlMzHLKNCl0dIkNxw XyckrSqtgRnmAAMEnrv5iyOK5QxypbiRjseY7jvpUVYA9D0WinydjImbXU+Zybqz43e7TQw7QNtGN o6E84KR64KSZvU0RmKHQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hEum5-0000yn-Jg; Fri, 12 Apr 2019 11:55:17 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hEum2-0000yR-TU for linux-arm-kernel@lists.infradead.org; Fri, 12 Apr 2019 11:55:16 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B4B23374; Fri, 12 Apr 2019 04:55:13 -0700 (PDT) Received: from fuggles.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A17973F718; Fri, 12 Apr 2019 04:55:12 -0700 (PDT) Date: Fri, 12 Apr 2019 12:55:07 +0100 From: Will Deacon To: Vincenzo Frascino Subject: Re: [PATCH v3 1/4] arm64: compat: Alloc separate pages for vectors and sigpage Message-ID: <20190412115507.GA28260@fuggles.cambridge.arm.com> References: <20190402162757.13491-1-vincenzo.frascino@arm.com> <20190402162757.13491-2-vincenzo.frascino@arm.com> <20190410181017.GA25618@fuggles.cambridge.arm.com> <7a4e4984-5448-5b93-5270-5b27abbec7f7@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7a4e4984-5448-5b93-5270-5b27abbec7f7@arm.com> User-Agent: Mutt/1.11.1+86 (6f28e57d73f2) () X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190412_045514_964492_DB5EA439 X-CRM114-Status: GOOD ( 33.21 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Catalin Marinas , linux@armlinux.org.uk, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org [+rmk] On Fri, Apr 12, 2019 at 12:08:30PM +0100, Vincenzo Frascino wrote: > On 10/04/2019 19:10, Will Deacon wrote: > > On Tue, Apr 02, 2019 at 05:27:54PM +0100, Vincenzo Frascino wrote: > >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > >> index 5d9ce62bdebd..07c873fce961 100644 > >> --- a/arch/arm64/include/asm/processor.h > >> +++ b/arch/arm64/include/asm/processor.h > >> @@ -78,9 +78,9 @@ > >> #endif /* CONFIG_ARM64_FORCE_52BIT */ > >> > >> #ifdef CONFIG_COMPAT > >> -#define AARCH32_VECTORS_BASE 0xffff0000 > >> +#define AARCH32_KUSER_BASE 0xffff0000 > >> #define STACK_TOP (test_thread_flag(TIF_32BIT) ? \ > >> - AARCH32_VECTORS_BASE : STACK_TOP_MAX) > >> + AARCH32_KUSER_BASE : STACK_TOP_MAX) > >> #else > >> #define STACK_TOP STACK_TOP_MAX > >> #endif /* CONFIG_COMPAT */ > >> diff --git a/arch/arm64/include/asm/signal32.h b/arch/arm64/include/asm/signal32.h > >> index 81abea0b7650..58e288aaf0ba 100644 > >> --- a/arch/arm64/include/asm/signal32.h > >> +++ b/arch/arm64/include/asm/signal32.h > >> @@ -20,8 +20,6 @@ > >> #ifdef CONFIG_COMPAT > >> #include > >> > >> -#define AARCH32_KERN_SIGRET_CODE_OFFSET 0x500 > >> - > >> int compat_setup_frame(int usig, struct ksignal *ksig, sigset_t *set, > >> struct pt_regs *regs); > >> int compat_setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, > >> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c > >> index cb7800acd19f..3846a1b710b5 100644 > >> --- a/arch/arm64/kernel/signal32.c > >> +++ b/arch/arm64/kernel/signal32.c > >> @@ -379,6 +379,7 @@ static void compat_setup_return(struct pt_regs *regs, struct k_sigaction *ka, > >> compat_ulong_t retcode; > >> compat_ulong_t spsr = regs->pstate & ~(PSR_f | PSR_AA32_E_BIT); > >> int thumb; > >> + void *sigreturn_base; > >> > >> /* Check if the handler is written for ARM or Thumb */ > >> thumb = handler & 1; > >> @@ -399,12 +400,12 @@ static void compat_setup_return(struct pt_regs *regs, struct k_sigaction *ka, > >> } else { > >> /* Set up sigreturn pointer */ > >> unsigned int idx = thumb << 1; > >> + sigreturn_base = current->mm->context.vdso; > >> > >> if (ka->sa.sa_flags & SA_SIGINFO) > >> idx += 3; > >> > >> - retcode = AARCH32_VECTORS_BASE + > >> - AARCH32_KERN_SIGRET_CODE_OFFSET + > >> + retcode = ptr_to_compat(sigreturn_base) + > >> (idx << 2) + thumb; > >> } > >> > >> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c > >> index 2d419006ad43..16f8fce5c501 100644 > >> --- a/arch/arm64/kernel/vdso.c > >> +++ b/arch/arm64/kernel/vdso.c > >> @@ -1,5 +1,7 @@ > >> /* > >> - * VDSO implementation for AArch64 and vector page setup for AArch32. > >> + * VDSO implementation for AArch64 and for AArch32: > >> + * AArch64: vDSO implementation contains pages setup and data page update. > >> + * AArch32: vDSO implementation contains sigreturn and kuser pages setup. > >> * > >> * Copyright (C) 2012 ARM Limited > >> * > >> @@ -53,61 +55,126 @@ struct vdso_data *vdso_data = &vdso_data_store.data; > >> /* > >> * Create and map the vectors page for AArch32 tasks. > >> */ > >> -static struct page *vectors_page[1] __ro_after_init; > >> +/* > >> + * aarch32_vdso_pages: > >> + * 0 - kuser helpers > >> + * 1 - sigreturn code > >> + */ > >> +#define C_VECTORS 0 > > > > C_KUSER might make more sense, and is consistent with AARCH32_KUSER_BASE. > > > > C_VECTORS seems consistent with the name of the page it refers to. Ok, then why change AARCH32_VECTORS_BASE? ;) > >> -int aarch32_setup_vectors_page(struct linux_binprm *bprm, int uses_interp) > >> +static int aarch32_kuser_helpers_setup(struct mm_struct *mm) > >> { > >> - struct mm_struct *mm = current->mm; > >> - unsigned long addr = AARCH32_VECTORS_BASE; > >> - static const struct vm_special_mapping spec = { > >> - .name = "[vectors]", > >> - .pages = vectors_page, > >> + void *ret; > >> + > >> + /* The kuser helpers must be mapped at the ABI-defined high address */ > >> + ret = _install_special_mapping(mm, AARCH32_KUSER_BASE, PAGE_SIZE, > >> + VM_READ | VM_EXEC | > >> + VM_MAYREAD | VM_MAYEXEC, > > > > How come you don't need VM_MAYWRITE here... > > > > This is to keep it consistent with what the arm (32 bit) implementation does. > > My understanding is that the kuser code is executed in user mode for efficiency > reasons but it is too close to the kernel to be implemented in user libraries > and that the kernel can change its internal implementation from version to > version as far as it guarantees the "interface" (entry points and results). > Based on this gdb should not need to put a breakpoint inside the kuser helpers code. Hmm, but couldn't you apply the same reasoning to the sigpage? [also, talking to Russell, he makes the very good point that you can't CoW the page containing the vectors because that would give userspace control over the vectors themselves!] > And if we consider as well that the fixed address nature of the helpers could be > used from ROP authors during the creation of exploits probably we want to > prevent gdb to set a breakpoint there hence the proposed patch does not contain > VM_MAYWRITE. I'm not sure I buy the ROP angle... you need to GUP the thing to get the write to happen. Maybe you could do it with a futex or something, but I'm also not sure that's really a viable attack. > I had a look to arm implementation and it seems the it defines the vector page > as READONLY_EXEC and there is no VM_MAYWRITE in the vm_flags. > > I could extend the comment accordingly. I initially thought that the gate VMA would mean that VM_MAYWRITE was implicit for GUP, but actually that's handled explicitly in get_gate_page(): /* user gate pages are read-only */ if (gup_flags & FOLL_WRITE) return -EFAULT; so yes, I agree with you that this is consistent with 32-bit. What I'm not sure about is why we need to CoW the sigpage. But I suppose being compatible with 32-bit is the aim of the game, so this is all moot. > >> + /* > >> + * VM_MAYWRITE is required to allow gdb to Copy-on-Write and > >> + * set breakpoints. > >> + */ > >> ret = _install_special_mapping(mm, addr, PAGE_SIZE, > >> - VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYEXEC, > >> - &spec); > >> + VM_READ | VM_EXEC | VM_MAYREAD | > >> + VM_MAYWRITE | VM_MAYEXEC, > >> + &aarch32_vdso_spec[C_SIGPAGE]); > > > > ... but you introduce it here?Also, shouldn't this be a separate change > > so it can be treated as a fix? > > This is again to keep it consistent with what the arm implementation does. > > Since the separation (vectors/sigpage) has been added in this patch, I am not > sure on how we can treat this change as a separate patch, at least based on what > I mentioned above. Could you please explain? Sorry, I was getting ahead of myself with that. If we need to add VM_MAYWRITE to the compat kuser helpers (which we could safely do on 64-bit), then we could do it as a fix. However, we don't need to do that do please ignore my comment above. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel