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 49DA0E77184 for ; Thu, 19 Dec 2024 18:26:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BADC26B007B; Thu, 19 Dec 2024 13:26:23 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B5D8F6B0082; Thu, 19 Dec 2024 13:26:23 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9D6F06B0083; Thu, 19 Dec 2024 13:26:23 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 7FCA06B007B for ; Thu, 19 Dec 2024 13:26:23 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id EDB9942A09 for ; Thu, 19 Dec 2024 18:26:22 +0000 (UTC) X-FDA: 82912537596.10.F6F2AEE Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by imf27.hostedemail.com (Postfix) with ESMTP id 3BCAB40010 for ; Thu, 19 Dec 2024 18:25:44 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=YA9fsH8W; spf=pass (imf27.hostedemail.com: domain of charlie@rivosinc.com designates 209.85.216.45 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=1734632746; 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=hMRnvozbssngVqbSknpjzbx2RK+JVnD39zwZDG+y2n8=; b=lhUg+5/fQVwC5coNgs4mXOJetriNWZ6jyXdE/UKjnqUAqonXB2CPM7o99tVBvGBGbZvjk6 p1JiryHDI6ugYKBcMabZ9WD3L3LcQR/YuPEpYLeQ6mZggjkDy8v+LN6O5lGBvUXXFt1z4A RLKMc/Dmsm8OWmX6AiFcaSrQ57pZZjM= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=YA9fsH8W; spf=pass (imf27.hostedemail.com: domain of charlie@rivosinc.com designates 209.85.216.45 as permitted sender) smtp.mailfrom=charlie@rivosinc.com; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1734632746; a=rsa-sha256; cv=none; b=K16GoS0zoqaFipA4vsYFqSwihi3rs4GibI17HUyEHn9GHu9tvnbX2MvJWPwihbkmxOeyR4 3Jmn3jMhm+/5oXsyHuQ1hhV7e2yL+gqpvrGOOnfh3Kh0leaLYiUSVBK5GXxG0bC8Rbe2FP PGu0ExbRmrfntanYhsvwE8kc2UxYVaM= Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2ef89dbd8eeso743760a91.0 for ; Thu, 19 Dec 2024 10:26:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1734632779; x=1735237579; 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=hMRnvozbssngVqbSknpjzbx2RK+JVnD39zwZDG+y2n8=; b=YA9fsH8WJrsKdNYgjPs7FhZWWOixPYOf7PEJdZCtu/Yqxm0UQxPIA0jB3XvQ4UyZsp BQIjZMzSFYIMLey8AbkVH5pHg3WNNE/cZTsfpymVx2aJBn62k6BjB2QL79E8McKdRnhK EwIgII6CUOT6pqk9o8D7gC4Spbj0DJAJihG1vv4leym4AyhLX+yQqtodB7beR+k47pJW 3HROCyLSNkzx6HmkHhVmMDIMiHINwtmDKbwRswpCtuJN2X8+Gw8VAqjxE/BEv52GbTKF rnue4O/gm5ahLQYjGsGGd6sIPFSwLcGqQUgBkYFU8kjaWiEpQW0UZbBLzfq2cMVPVoOt Ogeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734632779; x=1735237579; 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=hMRnvozbssngVqbSknpjzbx2RK+JVnD39zwZDG+y2n8=; b=FDytKhgjWrAN/OVsOhFdfXUHHUV1R0p1WAPZwXEJTD3RjaPv4R9TwOxWMb2cT3BfPo cVPTTQKLpnBMeU0ZNiR3TmRD7Qly3bw60Eg0KKTtOxxvehE4JEBAiEFW3BXVRDkhsPon QZN3YkQucUKwmDcDxrbPI+ZhLZ16waS6dm/Dz7ItBkBBgi8hdP71DDLJSTi3c5o1lTI8 /gd694NjTCze+fjngPGNX1GHNUlzwM60U2WYxi4FC4KLx/czJ1irUTz24BCKd0ElM9Tw Yi8uaeh+KghseuecvQeHEgSgwt8AnPBd6ObYht5bc3Gz2I6lfOHzx/K+2PDZDTT7mpgU evtA== X-Forwarded-Encrypted: i=1; AJvYcCUy+sEo5QF6NVZmpSR45DxAQijloAgX+TPafN/SqW8YGuPm4M5+8YwOEFtap+PxHluAcPnwC+jsaw==@kvack.org X-Gm-Message-State: AOJu0Yzzp26dFk5kednKWQ2A4sI19gYegbKwLJWGKld+nEkH7PVg1zmJ XymkjgABngJn/jpu0Pnzar+sBT0QsK78xePp0rRWHVyRLZHzN8nj9OQ8PLxiYps= X-Gm-Gg: ASbGncuv/F4HVhC3QvJMZIggrEJsdjhdVwObEtbpre63POO3ciGx0vdpy1H4UE4hvkD 7gPdctSbtSzl0cbjep4Esn2f+s8T+rDlyYbyX02c/Uxyd+KMDZKKIjCNWNMooRgJvM9uZ+nilmb A4uX+PX4FEu9ab8KDv0bq7uPcBIhVnUGOWpXFEL7w03OTWNgxC2jtB0jPR4Z76Kr1YeDjKl/BBt YFc4cuXgPcvv8PliQRHnf+EPZuleUiLKMwAF0NEhWemg27ixABh X-Google-Smtp-Source: AGHT+IFsWgJOva12zvp6tp180q3OiFLZ0d0TjIkVYwvTLN5q8JRYcM/SHtSfXHGK+OHbcvE+dj549w== X-Received: by 2002:a17:90b:2c84:b0:2ee:bbe0:98c6 with SMTP id 98e67ed59e1d1-2f452dfcb28mr13994a91.8.1734632779465; Thu, 19 Dec 2024 10:26:19 -0800 (PST) Received: from ghost ([2601:647:6700:64d0:1e6a:6188:cfff:248e]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2f447883b0csm1887523a91.42.2024.12.19.10.26.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Dec 2024 10:26:18 -0800 (PST) Date: Thu, 19 Dec 2024 10:26:15 -0800 From: Charlie Jenkins To: Andrew Jones Cc: Celeste Liu , 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 , Felix Yan , Ruizhe Pan , Shiqi Zhang , Guo Ren , Yao Zi , Han Gao , 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241203-55c76715e16422ddaf8d7edf@orel> X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 3BCAB40010 X-Rspam-User: X-Stat-Signature: ayof5crsafnn8d8izorfex8sbzr7ui7i X-HE-Tag: 1734632744-98790 X-HE-Meta: U2FsdGVkX18aJEqLp4ewOhbCUb29JoPqo+GTwXe5h+HxwKnOUC5nTibnB4ZqZCDGvutJAzDp0c0yXymq72r+ttLOHpS9X2p3C7EZRP5v97BG+dhY4SlGIat7FQYDcwQeZAyhKBNFO/PcNVguTpueuwCz+WjTRDZXORZwktfUQjZpI1Qg8p8ivpLNZHXRIfQirVLPsqvSCFl0EYWmPq3YrDkjXg8KkWmdH/tPpBKgxe6EheyHBTdkEQfjdSGA5EF3kfL/EBbg4K41bka5JhKB+pVYYv72p+GbZlcGRDCRJCrarSvu0yRJp3C0qDyo/p0KHTg0AJWRi9aQpbFTOnzvSHJeoO3+3R4yEX6kBaHUPyeCF0EEOaFoXiMq5wAyQDc6D8PMNdXt7iAkYEYpzSuGAxOzh0nuMIe+60k1AxU45/xJV8R79CdYKv+n3tdmFPgq0ik5BYFCVvuHGcBEweSQ1B1De41TxsjaDFk/2Gs6Rr3XUeLuA9nF3rY9xeYElgnpZ7hWvMJCcFg/vnoVMW7usdjtdMOPxv+IQvMmw+xlbwS2QPDxs+JmWnLoeFsnKvMSa/i+1giYTa6uz2gVRI10o176qmvrx3RbG0+Y2b/uA7meNGXikjPMBzEe0o+7uBtq03ruC+tXfUpZXOlRHltghyptliwcYWZHQs6sMG8MsnuXkk7XdgAiURr4pWBhfZPRdtYcqA4fPABZdtwZ08Skgdk4xC57l6FoP3yMy/JvxmLmKip7M0kdbxD6x7LBUJoqt8cA9+3V0Ss7NELFvaCr0FpbX7JPe9xfXdJCz/b+U+0sueYlQKsqfi3bYMm8S8kPPcjwgTJT54UhEHwHZ/+HDVNVCAotUE3tZXqXbFCsdSc9OnWzVrJKCVkaNTeL5gszkcYCEofwu+yisN5TOhzbJLy/xT6gwKx9x/V8E3Yg+MGhDknjTuBlKOHf3nuTda7DyzsDIcet7e4Ym/Lwefr 22gna4kM irWqrl9aH0HSisyeM0ofyXI+SIQVCoDepc8G9LZzPShV5gusUEWDyCKIbRjDilxG2RW2NTatJ/+w1AaJI9t9kCeTWOkSC3F2OZ7q5+k+wTP7r/Co5dnbEI7m4KKsPGTohy5HT5P9+FiTyJoU0W8Csu/cN0JWwaTzpvUN8pzgsIFV/0D9La0vENvpfnrMHdfzIaotoKc9jNEg0tHOwAIdhY0O/MAUppOhNeQkWwVm9R/CfWxVAUpTz5QMUQnwman3064OgGbU30mUIURSSQjZM3MoFP572r3LzMyQapNnavWX4X2lsqBPGvyywVdegWH8hKKixQVd4jnCaiDQR5hK+UeXucLNJ6JN//txK4/IKWHgTBQIloT+55RXICqDeHIkRv/5D6EyvPk3wUHls+wX8Ew9/X2EF7ecKKi99klb+o7Vy2DnLrxwsOo2QpIESh96iYYwuj8+9mCCs+x7AE8L4+2N+7AP+sNpx8PnPuYWqGFXxewE= X-Bogosity: Ham, tests=bogofilter, spamicity=0.009010, 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 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? - Charlie > > + EXPECT_EQ(A0_NEW, result); > > +} > > + > > +TEST_HARNESS_MAIN > > > > -- > > 2.47.0 > > > > Thanks, > drew