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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id F0087E77184 for ; Thu, 19 Dec 2024 21:36:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B37046B007B; Thu, 19 Dec 2024 16:36:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id ABD4D6B0082; Thu, 19 Dec 2024 16:36:58 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 937326B0083; Thu, 19 Dec 2024 16:36:58 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 726706B007B for ; Thu, 19 Dec 2024 16:36:58 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 205101203AD for ; Thu, 19 Dec 2024 21:36:58 +0000 (UTC) X-FDA: 82913017362.08.A0408B3 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) by imf11.hostedemail.com (Postfix) with ESMTP id 63ACF40010 for ; Thu, 19 Dec 2024 21:36:25 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=OHktrQGO; spf=pass (imf11.hostedemail.com: domain of charlie@rivosinc.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=charlie@rivosinc.com; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1734644201; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=sBYqbJyIvcJfLc67nzPKYdjSB+90GgtMbY2G3flDnUI=; b=UmMofdxAqPktYwnYn+54el1rj4ocG4JFI5VJdVWekXZtUKGD6N9tcv11D1vXDj22Z7eHpq B3nO73Slo8Mbmgf2gOMxxqip4TiPXPm4bzcTbQaU1b0ttmmgtAwKFB9C7xqMZ7LlSmPbYQ CYnOgHsHHCmLj9faTHnD33ZE1zk10V8= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=OHktrQGO; spf=pass (imf11.hostedemail.com: domain of charlie@rivosinc.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=charlie@rivosinc.com; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1734644201; a=rsa-sha256; cv=none; b=oZdavaxYKX0D8E0UrM80WQNZH2HYTsFiHjJjcMGr/3giZnmZUOZCxcaCy+X02bK1Dby3O6 2WCOIG/mTML3nr5nDpnghn6JJFD5w2EFcOGG5ljIfgCGtbNU4MTUSwOycLfVbv7yg9/quL dlHwbYvn+bobbt+9G9O81RPAxShvpOo= Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-2164b662090so11549615ad.1 for ; Thu, 19 Dec 2024 13:36:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1734644215; x=1735249015; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=sBYqbJyIvcJfLc67nzPKYdjSB+90GgtMbY2G3flDnUI=; b=OHktrQGOFiJij9Nhq5j95yt2EGlfLq5IIXz1eP/XtcBUuwaj6wYYkWHYpx2yF6E938 SSSvirMjZGL2VX3QqA/sCfl/S5A9NGTBnzGSsSJmiI2hs38BGA3YJ/VBFD6US7ka3dvC EiTRgE99Sm/I/twcyfU3v13QRsU/5Azt75Xn630IKrjr0+23VvzekNje77iPjqa2Gyhp F5aWnK53IdstXQBQWtKdQ8HmdARElz9a7ZTO3KhGv8dT/KBHxSBHz8sOApXQJP8op7pj lX5CpX/iAqFqO/LZNAEl+53Repieu9XuG+sMQzVMFzpzvGZ29kd5uM37vIAv8UyZ2oDz cyaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734644215; x=1735249015; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=sBYqbJyIvcJfLc67nzPKYdjSB+90GgtMbY2G3flDnUI=; b=UhFWDV4RvHMGURg9TH3Jo6E9DrkTXRoxpNSn0gFyCUBSAGbtbsQ8CqZ3ioWORybsBJ L3bIn4+nDgexfsMEuAWKG4cIbsaXVYv2FbfJD0T83HvJNLKEoggNTKp4N3F6fHNN84js zoQcSjP+i71D4rkiFyoUn4bgcE1EbfeowxL1+r5EjVL0AxDOm2QEL5yx3dzPaWY4deqV Ay3PUwGEi3UvTVxNpdU6fs5GZLfiTtelwmyc3sgtuZ6oSXQKRvzF2zeSlmNt4/p8JBdN L/frhFUAVNwqccy8JSLcGNLJgvqeRKc3QoUeVmrQMh/FBPZx39pm9P4tPUp9Oq6ZA4/F Xo2Q== X-Forwarded-Encrypted: i=1; AJvYcCUEz83n/+6SOP4PQOn8larhm4+3ZepHGY2H1tVyqXdRAg3WlJaDWgB4aVvk8rcYFDc4waN1O1Cfnw==@kvack.org X-Gm-Message-State: AOJu0Yw1mtjbhLKryke3fYf8GW7Eufaj1bZ1Sm5z/WPnCHz/Wq53aorv Ci4uLL8WFRwCd8iJrRp0Gfds0QuKa8C2Nh5ytAnBGQer8EwK+ApGLkrEQFVDZz5ISj0E5dgPZcu w X-Gm-Gg: ASbGncse/JPgPdWf2IE2oPNKsaVQpBdwI0oSgPTYMJgF5WYbrWFMvJqw2dy1YVODhn0 46Dcovh4j9CkwfuyZn/k8cdt9UpqKiLgqBlWUm6sCy03SdQ5DUmWK1XsrtTF7w1twBX7raDiI+Q QPNmHJ9IBmQ2v6FgojC2wilNBzm9rZIcLMDzo87kqZ+TEitxGg73eNqI6zucRE7bNWjOKTCxzvJ f1duTFRl5YVOzp3hoyYxWOSP22OjwObLAMHVQmbwR9le1E= X-Google-Smtp-Source: AGHT+IG/Dgc/5kYcDlkg099vZ9/LAsV1QDOIoLm+mwtiNyuB7pILZ0WR6EgDxOuwJUotkZK+n+l86A== X-Received: by 2002:a17:90a:d64d:b0:2ee:b2fe:eeee with SMTP id 98e67ed59e1d1-2f452e3021cmr942885a91.15.1734644214713; Thu, 19 Dec 2024 13:36:54 -0800 (PST) Received: from ghost ([50.145.13.30]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2f2ed644d87sm4166935a91.27.2024.12.19.13.36.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Dec 2024 13:36:54 -0800 (PST) Date: Thu, 19 Dec 2024 13:36:51 -0800 From: Charlie Jenkins To: Celeste Liu Cc: Andrew Jones , Oleg Nesterov , Paul Walmsley , Palmer Dabbelt , Albert Ou , Eric Biederman , Kees Cook , Shuah Khan , Alexandre Ghiti , "Dmitry V. Levin" , Andrea Bolognani , =?iso-8859-1?Q?Bj=F6rn_T=F6pel?= , Thomas Gleixner , Ron Economos , Quan Zhou , Guo Ren , linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, stable@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification Message-ID: References: <20241203-riscv-new-regset-v2-0-d37da8c0cba6@coelacanthus.name> <20241203-riscv-new-regset-v2-2-d37da8c0cba6@coelacanthus.name> <20241203-55c76715e16422ddaf8d7edf@orel> <50a62291-5ae1-47b0-8f64-a42271820789@coelacanthus.name> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50a62291-5ae1-47b0-8f64-a42271820789@coelacanthus.name> X-Rspamd-Queue-Id: 63ACF40010 X-Stat-Signature: iw4gq5eemnzetohzhx7o86rkpautgdwq X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1734644185-302992 X-HE-Meta: U2FsdGVkX19GIOYQ7yyfgg5WJdtSukbjKBuXGH55w5Fd34CcWeM1/FO9ULsLhDL7M2L56/ExBuWbih65ycuDmipumI27Wd77BQsR3/e0O39c6QGLpO5vmi3E2nB7drKFPDzZnLyB8D8q3Y3W03dZi/Bo3DekHSeEKZ3A69r2D80hoD5hVMRZWKxbyfHvUzvHUCsP2d2OCf3YgjoB1CW6cQtyPD6GP3AbXJdW5OEWUl+aVoMEdySRAvXQw2RGo2z2oUd6FDT/MHCu5I2UkSI/6iP3+9ZFq3iO9xxYF42vhqEyoKkd8e6IKzwqMco0rSG2uynwsAIDplzwW31l0SnDFtSxoDCm5jVzNR48IKwUkhhgqsOYxozx+n5d3vZ8qj1j+2dOGWwUO35roMhLjoUmxeDVKYbc3rliSND7tPPtQSeYy90VO8h0+k54hTeWSGU6mKYAVVWf+3uveijBp/tfdyxufAGuJMpQNPeLj72j7KjS+VHpOQ0fMMR8eIwcZwmXBekElu+dRx7qHf5fawQr/gpJ9vTKUq/CRRxPZTASvvTStPmSVmvcDGY2LL+2/rJHKHR6rpPWOvqfwyoLjyXlnR9Ww2Y2tR5DBb1VA2KaPOkF3G7eTT6QarvBd+f4ng6R3hAsm36W9uosvpJNkgYZFmhmBxvspzGdp770ps6j4ZpDm7Pg6ac3wmdCB9gEXHjEn39J+/ks4BQ1YBktUHX5aKcjJxWj7/gPBwR2c4jNJYQW2LsXbVDrN6jvJeeGGa2fgTGtaW152AaxFJTBFdrwCN+3feAI9bG9peRzjUakl8xi+RpJsglF6pRnPCthHaFCIbfBILE3k585QDqA048ouK31Ha5Hytxk1+qBkLX+XCJrLqZ7Nih40IloKXyOk/TcYc6+YoAOIIEc5Wq6f6z5IB7xk5sJ88xja9qN2M4i6QgGXFwlhzQySJ/hEAy1fbhRokABJ9S5XUUZpan//V2 gNchO7N0 2VKLBxY0V6C0xcLByao64sm1XA5bpbLsM7aayLCIVCyRix9r08gba/gGmeOGXA5baX2h9dVYSc1E9U+GC86EBva/OXagu+mRdRDY0sWzixgBWuiiH6wh0z0gA6nbYV0w8XD4BwUD9+Y8lBdt2ns6hm1XS1DYG/tI3I7sXKyuTaZdBgM2FtkyRQJb9h+LNTvLzWvRwKzsoyrIFXUBlSC59Du/+ls+PNY1d6EcIThuy92RSIK6O7P4+WfrELxn8aXU56yQk/0c/JO6O2npZVf1BRmoBx+/MuVEc8BGhv2fqPjXIxr41CdzCeSsrif0DRqml66NTCdY/kBFjXEgTYALT6UtYK5Ob6Riee+kuVCfj2nJXKB1K9jBk6PRFLMXgui/Sl/ovyfTrxQVvIO/m4oZq7tiRa2G+MUSCcDAZuxHJL0ddsUz0TapgUScMbEsi06V4khFM X-Bogosity: Ham, tests=bogofilter, spamicity=0.000452, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Dec 20, 2024 at 05:29:45AM +0800, Celeste Liu wrote: > > On 2024-12-20 02:26, Charlie Jenkins wrote: > > On Tue, Dec 03, 2024 at 01:55:07PM +0100, Andrew Jones wrote: > >> On Tue, Dec 03, 2024 at 05:30:05PM +0800, Celeste Liu wrote: > >>> From: Charlie Jenkins > >>> > >>> This test checks that orig_a0 allows a syscall argument to be modified, > >>> and that changing a0 does not change the syscall argument. > >>> > >>> Cc: stable@vger.kernel.org > >>> Co-developed-by: Quan Zhou > >>> Signed-off-by: Quan Zhou > >>> Co-developed-by: Celeste Liu > >>> Signed-off-by: Celeste Liu > >>> Signed-off-by: Charlie Jenkins > >>> --- > >>> tools/testing/selftests/riscv/abi/.gitignore | 1 + > >>> tools/testing/selftests/riscv/abi/Makefile | 5 +- > >>> tools/testing/selftests/riscv/abi/ptrace.c | 134 +++++++++++++++++++++++++++ > >>> 3 files changed, 139 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore > >>> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644 > >>> --- a/tools/testing/selftests/riscv/abi/.gitignore > >>> +++ b/tools/testing/selftests/riscv/abi/.gitignore > >>> @@ -1 +1,2 @@ > >>> pointer_masking > >>> +ptrace > >>> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile > >>> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644 > >>> --- a/tools/testing/selftests/riscv/abi/Makefile > >>> +++ b/tools/testing/selftests/riscv/abi/Makefile > >>> @@ -2,9 +2,12 @@ > >>> > >>> CFLAGS += -I$(top_srcdir)/tools/include > >>> > >>> -TEST_GEN_PROGS := pointer_masking > >>> +TEST_GEN_PROGS := pointer_masking ptrace > >>> > >>> include ../../lib.mk > >>> > >>> $(OUTPUT)/pointer_masking: pointer_masking.c > >>> $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ > >>> + > >>> +$(OUTPUT)/ptrace: ptrace.c > >>> + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ > >>> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c > >>> new file mode 100644 > >>> index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556 > >>> --- /dev/null > >>> +++ b/tools/testing/selftests/riscv/abi/ptrace.c > >>> @@ -0,0 +1,134 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-only > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> + > >>> +#include "../../kselftest_harness.h" > >>> + > >>> +#define ORIG_A0_MODIFY 0x01 > >>> +#define A0_MODIFY 0x02 > >>> +#define A0_OLD 0x03 > >>> +#define A0_NEW 0x04 > >> > >> Shouldn't A0_OLD and A0_NEW set more bits, since 3 and 4 aren't very > >> unique (we could have those values by accident)? And should we include > >> setting bits over 31 for 64-bit targets? > >> > >>> + > >>> +#define perr_and_exit(fmt, ...) \ > >>> + ({ \ > >>> + char buf[256]; \ > >>> + snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n", \ > >>> + __func__, __LINE__, ##__VA_ARGS__); \ > >>> + perror(buf); \ > >>> + exit(-1); \ > >>> + }) > >> > >> Can we use e.g. ksft_exit_fail_perror() instead? I'd prefer we try to > >> consolidate testing/selftests/riscv/* tests on a common format for > >> errors and exit codes and we're already using other kselftest stuff. > >> > >>> + > >>> +static inline void resume_and_wait_tracee(pid_t pid, int flag) > >>> +{ > >>> + int status; > >>> + > >>> + if (ptrace(flag, pid, 0, 0)) > >>> + perr_and_exit("failed to resume the tracee %d\n", pid); > >>> + > >>> + if (waitpid(pid, &status, 0) != pid) > >>> + perr_and_exit("failed to wait for the tracee %d\n", pid); > >>> +} > >>> + > >>> +static void ptrace_test(int opt, int *result) > >>> +{ > >>> + int status; > >>> + pid_t pid; > >>> + struct user_regs_struct regs; > >>> + struct iovec iov = { > >>> + .iov_base = ®s, > >>> + .iov_len = sizeof(regs), > >>> + }; > >>> + > >>> + unsigned long orig_a0; > >>> + struct iovec a0_iov = { > >>> + .iov_base = &orig_a0, > >>> + .iov_len = sizeof(orig_a0), > >>> + }; > >>> + > >>> + pid = fork(); > >>> + if (pid == 0) { > >>> + /* Mark oneself being traced */ > >>> + long val = ptrace(PTRACE_TRACEME, 0, 0, 0); > >>> + > >>> + if (val) > >>> + perr_and_exit("failed to request for tracer to trace me: %ld\n", val); > >>> + > >>> + kill(getpid(), SIGSTOP); > >>> + > >>> + /* Perform exit syscall that will be intercepted */ > >>> + exit(A0_OLD); > >>> + } > >>> + > >>> + if (pid < 0) > >>> + exit(1); > >> > >> This unexpected error condition deserves a message, so I'd use > >> ksft_exit_fail_perror() here. > >> > >>> + > >>> + if (waitpid(pid, &status, 0) != pid) > >>> + perr_and_exit("failed to wait for the tracee %d\n", pid); > >>> + > >>> + /* Stop at the entry point of the syscall */ > >>> + resume_and_wait_tracee(pid, PTRACE_SYSCALL); > >>> + > >>> + /* Check tracee regs before the syscall */ > >>> + if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov)) > >>> + perr_and_exit("failed to get tracee registers\n"); > >>> + if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov)) > >>> + perr_and_exit("failed to get tracee registers\n"); > >>> + if (orig_a0 != A0_OLD) > >>> + perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0); > >>> + > >>> + /* Modify a0/orig_a0 for the syscall */ > >>> + switch (opt) { > >>> + case A0_MODIFY: > >>> + regs.a0 = A0_NEW; > >>> + break; > >>> + case ORIG_A0_MODIFY: > >>> + orig_a0 = A0_NEW; > >>> + break; > >>> + } > >>> + > >>> + if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov)) > >>> + perr_and_exit("failed to set tracee registers\n"); > >>> + > >>> + /* Resume the tracee */ > >>> + ptrace(PTRACE_CONT, pid, 0, 0); > >>> + if (waitpid(pid, &status, 0) != pid) > >>> + perr_and_exit("failed to wait for the tracee\n"); > >>> + > >>> + *result = WEXITSTATUS(status); > >>> +} > >>> + > >>> +TEST(ptrace_modify_a0) > >>> +{ > >>> + int result; > >>> + > >>> + ptrace_test(A0_MODIFY, &result); > >>> + > >>> + /* The modification of a0 cannot affect the first argument of the syscall */ > >>> + EXPECT_EQ(A0_OLD, result); > >> > >> What about checking that we actually set regs.a0 to A0_NEW? We'd need > >> A0_NEW to be more unique than 4, though. > >> > >>> +} > >>> + > >>> +TEST(ptrace_modify_orig_a0) > >>> +{ > >>> + int result; > >>> + > >>> + ptrace_test(ORIG_A0_MODIFY, &result); > >>> + > >>> + /* Only modify orig_a0 to change the first argument of the syscall */ > >> > >> If we run ptrace_modify_a0 first then we've already set regs.a0 to A0_NEW > >> and can't check with this test that we don't set it to A0_NEW. We should > >> probably have two different test values, one for regs.a0 and one for > >> orig_a0 and ensure on both tests that we aren't writing both. > >> > > > > Celeste, do you want to fix this up or are you waiting for me to? > > Sorry for delay. I was busy with household affairs in the past few weeks. > v3 will be sent tomorrow or the day after tomorrow. > > I am deeply sorry for this. No need to apologize! Just wanted to make sure you weren't expected me to update the test :) - Charlie > > > > > - Charlie > > > >>> + EXPECT_EQ(A0_NEW, result); > >>> +} > >>> + > >>> +TEST_HARNESS_MAIN > >>> > >>> -- > >>> 2.47.0 > >>> > >> > >> Thanks, > >> drew >