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=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 AD03BC433E0 for ; Thu, 25 Jun 2020 17:10:27 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5D81E20707 for ; Thu, 25 Jun 2020 17:10:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="NX2oHbHu" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5D81E20707 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D28386B0089; Thu, 25 Jun 2020 13:10:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CD74F8D0009; Thu, 25 Jun 2020 13:10:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BA07D6B0093; Thu, 25 Jun 2020 13:10:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0091.hostedemail.com [216.40.44.91]) by kanga.kvack.org (Postfix) with ESMTP id 98E686B0089 for ; Thu, 25 Jun 2020 13:10:26 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 2602397604 for ; Thu, 25 Jun 2020 17:10:26 +0000 (UTC) X-FDA: 76968372852.28.soap96_0c1034026e4e Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin28.hostedemail.com (Postfix) with ESMTP id 87B7710FBF for ; Thu, 25 Jun 2020 17:10:19 +0000 (UTC) X-HE-Tag: soap96_0c1034026e4e X-Filterd-Recvd-Size: 12218 Received: from mail-qk1-f195.google.com (mail-qk1-f195.google.com [209.85.222.195]) by imf29.hostedemail.com (Postfix) with ESMTP for ; Thu, 25 Jun 2020 17:10:18 +0000 (UTC) Received: by mail-qk1-f195.google.com with SMTP id 80so6051401qko.7 for ; Thu, 25 Jun 2020 10:10:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Sv+Z+ChUo+I3RTeltNTOvIPjX+YdP9Dt/bxhBMjeIjw=; b=NX2oHbHuJSF88sChYdb5kWXnWrsq09FOJ24qLE43ytGz4/cJ8L9bKsldX7P3g57gaq Bp0W7KZv4sw+Zgdh7vPSAnjbfl5yVRnVR8+fzlSXWam2v6J0sJaP4gYVju80YDtoIIhJ CfcvtdohmBIBEsgoQwy3CDwdBSiwkfKe2hPzinb6aI7VCU2Qwrmfdw3BJyZvzJOSXFnG y/r0CN45Ptymf5wbAFPBjZp+gRLFwKta/V6doy3YCTzm/kk2z7tjbtKlCFBbteu05ht0 y+1mnlK8Q/J4INCNQmrfst6Pq2Zwdhnpfnov3wKr7pfEZi9icDW4yv5ch9wx43EA915C oEfw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Sv+Z+ChUo+I3RTeltNTOvIPjX+YdP9Dt/bxhBMjeIjw=; b=hLoeiY6242M1qadLpDukL8HgHmtWFcVFtCiO8iXN0DelasN3+cfFqFgYK7ZDfBvgi2 NEuP3oHT6AaNWj5eTNJ1/Da7ev6AcGYeu/+C/ci/3L5fKDJ+idA9hd3zM+uAIzlKaN5v nj6CkO+1GrBlReeQnKV56UZHcxRB1+Krdd2oJ0YZYeLAUIaXpXJNxGyqSWCl0yBexetQ +M/vPBMuENgEivGT5Zu2csTmkr5z024oJc6YmS2UYfUjvBCaWBmotsHeMMVltaoQX0l2 x2Dbw3aa6hC2W9a3UPqCwVJ+ZcJIPa05yvSFUX2eM+gue0eVAODTs2KZo+agnPMG4z43 aXlQ== X-Gm-Message-State: AOAM531IZRdcsOF8m3E8ML61Ncp/dRHJLGXNiRXuu4d5ww5iP6skEHt7 dOu8oPRiMN0k0ISYx5zxy9bkFg== X-Google-Smtp-Source: ABdhPJy3gQ7LUHrsyNYWzzF1D2AFkm9lHohxPRRAQF1UkmF63cRwEiJM79VamDzyC+S9t+gdHeLGzg== X-Received: by 2002:a37:887:: with SMTP id 129mr30935950qki.52.1593105017959; Thu, 25 Jun 2020 10:10:17 -0700 (PDT) Received: from [192.168.0.185] ([191.249.225.93]) by smtp.gmail.com with ESMTPSA id 207sm6246921qki.134.2020.06.25.10.10.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 25 Jun 2020 10:10:15 -0700 (PDT) Subject: Re: [PATCH v5 19/25] arm64: mte: Add PTRACE_{PEEK,POKE}MTETAGS support To: Catalin Marinas , linux-arm-kernel@lists.infradead.org Cc: linux-mm@kvack.org, linux-arch@vger.kernel.org, Will Deacon , Dave P Martin , Vincenzo Frascino , Szabolcs Nagy , Kevin Brodsky , Andrey Konovalov , Peter Collingbourne , Andrew Morton , Alan Hayward , Omair Javaid References: <20200624175244.25837-1-catalin.marinas@arm.com> <20200624175244.25837-20-catalin.marinas@arm.com> From: Luis Machado Message-ID: <7fd536af-f9fa-aa10-a4c3-001e80dd7d7b@linaro.org> Date: Thu, 25 Jun 2020 14:10:10 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <20200624175244.25837-20-catalin.marinas@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 87B7710FBF X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 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: Hi Catalin, I have one point below I wanted to clarify regarding PEEKMTETAGS/POKEMTETAGS. But before that, I've pushed v2 of the MTE series for GDB here: https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/luisgpm/aarch64-mte-v2 That series adds sctlr and gcr registers to the NT_ARM_MTE (still using a dummy value of 0x407) register set. It would be nice if the Linux Kernel and the debuggers were in sync in terms of supporting this new register set. GDB assumes the register set exists if HWCAP2_MTE is there. So, if we want to adjust the register set, we should probably consider doing that now. That prevents the situation where debuggers would need to do another check to confirm NT_ARM_MTE is exported. I'd rather avoid that. What do you think? On 6/24/20 2:52 PM, Catalin Marinas wrote: > Add support for bulk setting/getting of the MTE tags in a tracee's > address space at 'addr' in the ptrace() syscall prototype. 'data' points > to a struct iovec in the tracer's address space with iov_base > representing the address of a tracer's buffer of length iov_len. The > tags to be copied to/from the tracer's buffer are stored as one tag per > byte. > > On successfully copying at least one tag, ptrace() returns 0 and updates > the tracer's iov_len with the number of tags copied. In case of error, > either -EIO or -EFAULT is returned, trying to follow the ptrace() man > page. > > Note that the tag copying functions are not performance critical, > therefore they lack optimisations found in typical memory copy routines. > > Signed-off-by: Catalin Marinas > Cc: Will Deacon > Cc: Alan Hayward > Cc: Luis Machado > Cc: Omair Javaid > --- > > Notes: > v4: > - Following the change to only clear the tags in a page if it is mapped > to user with PROT_MTE, ptrace() now will refuse to access tags in > pages not previously mapped with PROT_MTE (PG_mte_tagged set). This is > primarily to avoid leaking uninitialised tags to user via ptrace(). > - Fix SYM_FUNC_END argument typo. > - Rename MTE_ALLOC_* to MTE_GRANULE_*. > - Use uao_user_alternative for the user access in case we ever want to > call mte_copy_tags_* with a kernel buffer. It also matches the other > uaccess routines in the kernel. > - Simplify arch_ptrace() slightly. > - Reorder down_write_killable() with access_ok() in > __access_remote_tags(). > - Handle copy length 0 in mte_copy_tags_{to,from}_user(). > - Use put_user() instead of __put_user(). > > New in v3. > > arch/arm64/include/asm/mte.h | 17 ++++ > arch/arm64/include/uapi/asm/ptrace.h | 3 + > arch/arm64/kernel/mte.c | 139 +++++++++++++++++++++++++++ > arch/arm64/kernel/ptrace.c | 7 ++ > arch/arm64/lib/mte.S | 53 ++++++++++ > 5 files changed, 219 insertions(+) > > diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h > index c93047eff9fe..5fe9678d2e14 100644 > --- a/arch/arm64/include/asm/mte.h > +++ b/arch/arm64/include/asm/mte.h > @@ -5,6 +5,11 @@ > #ifndef __ASM_MTE_H > #define __ASM_MTE_H > > +#define MTE_GRANULE_SIZE UL(16) > +#define MTE_GRANULE_MASK (~(MTE_GRANULE_SIZE - 1)) > +#define MTE_TAG_SHIFT 56 > +#define MTE_TAG_SIZE 4 > + > #ifndef __ASSEMBLY__ > > #include > @@ -12,6 +17,10 @@ > #include > > void mte_clear_page_tags(void *addr); > +unsigned long mte_copy_tags_from_user(void *to, const void __user *from, > + unsigned long n); > +unsigned long mte_copy_tags_to_user(void __user *to, void *from, > + unsigned long n); > > #ifdef CONFIG_ARM64_MTE > > @@ -25,6 +34,8 @@ void mte_thread_switch(struct task_struct *next); > void mte_suspend_exit(void); > long set_mte_ctrl(unsigned long arg); > long get_mte_ctrl(void); > +int mte_ptrace_copy_tags(struct task_struct *child, long request, > + unsigned long addr, unsigned long data); > > #else > > @@ -54,6 +65,12 @@ static inline long get_mte_ctrl(void) > { > return 0; > } > +static inline int mte_ptrace_copy_tags(struct task_struct *child, > + long request, unsigned long addr, > + unsigned long data) > +{ > + return -EIO; > +} > > #endif > > diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h > index 06413d9f2341..758ae984ff97 100644 > --- a/arch/arm64/include/uapi/asm/ptrace.h > +++ b/arch/arm64/include/uapi/asm/ptrace.h > @@ -76,6 +76,9 @@ > /* syscall emulation path in ptrace */ > #define PTRACE_SYSEMU 31 > #define PTRACE_SYSEMU_SINGLESTEP 32 > +/* MTE allocation tag access */ > +#define PTRACE_PEEKMTETAGS 33 > +#define PTRACE_POKEMTETAGS 34 > > #ifndef __ASSEMBLY__ > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index 09cf76fc1090..3e08aea56e7a 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -4,14 +4,18 @@ > */ > > #include > +#include > #include > #include > #include > +#include > #include > #include > +#include > > #include > #include > +#include > #include > > void mte_sync_tags(pte_t *ptep, pte_t pte) > @@ -173,3 +177,138 @@ long get_mte_ctrl(void) > > return ret; > } > + > +/* > + * Access MTE tags in another process' address space as given in mm. Update > + * the number of tags copied. Return 0 if any tags copied, error otherwise. > + * Inspired by __access_remote_vm(). > + */ > +static int __access_remote_tags(struct task_struct *tsk, struct mm_struct *mm, > + unsigned long addr, struct iovec *kiov, > + unsigned int gup_flags) > +{ > + struct vm_area_struct *vma; > + void __user *buf = kiov->iov_base; > + size_t len = kiov->iov_len; > + int ret; > + int write = gup_flags & FOLL_WRITE; > + > + if (!access_ok(buf, len)) > + return -EFAULT; > + > + if (mmap_read_lock_killable(mm)) > + return -EIO; > + > + while (len) { > + unsigned long tags, offset; > + void *maddr; > + struct page *page = NULL; > + > + ret = get_user_pages_remote(tsk, mm, addr, 1, gup_flags, > + &page, &vma, NULL); > + if (ret <= 0) > + break; > + > + /* > + * Only copy tags if the page has been mapped as PROT_MTE > + * (PG_mte_tagged set). Otherwise the tags are not valid and > + * not accessible to user. Moreover, an mprotect(PROT_MTE) > + * would cause the existing tags to be cleared if the page > + * was never mapped with PROT_MTE. > + */ > + if (!test_bit(PG_mte_tagged, &page->flags)) { > + ret = -EOPNOTSUPP; > + put_page(page); > + break; > + } > + > + /* limit access to the end of the page */ > + offset = offset_in_page(addr); > + tags = min(len, (PAGE_SIZE - offset) / MTE_GRANULE_SIZE); > + > + maddr = page_address(page); > + if (write) { > + tags = mte_copy_tags_from_user(maddr + offset, buf, tags); > + set_page_dirty_lock(page); > + } else { > + tags = mte_copy_tags_to_user(buf, maddr + offset, tags); > + } > + put_page(page); > + > + /* error accessing the tracer's buffer */ > + if (!tags) > + break; > + > + len -= tags; > + buf += tags; > + addr += tags * MTE_GRANULE_SIZE; > + } > + mmap_read_unlock(mm); > + > + /* return an error if no tags copied */ > + kiov->iov_len = buf - kiov->iov_base; > + if (!kiov->iov_len) { > + /* check for error accessing the tracee's address space */ > + if (ret <= 0) > + return -EIO; > + else > + return -EFAULT; > + } > + > + return 0; > +} My understanding is that both the PEEKMTETAGS and POKEMTETAGS can potentially read/write less tags than requested, right? The iov_len field will be updated accordingly. So the ptrace caller would need to loop and make sure all the tags were read/written, right? I'm considering the situation where the kernel reads/writes 0 tags (when requested to read/write 1 or more tags) an error we can't recover from. So this may indicate a page without PROT_MTE or an invalid address. Does that make sense?