From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f179.google.com (mail-pl1-f179.google.com [209.85.214.179]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5CC0C1509BD for ; Thu, 19 Dec 2024 21:36:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.179 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734644218; cv=none; b=n523YpA0rGpS4cGy3d+p61ggU3WtMMLDgNyseIaMTIO92AUc++d0U6iKdI2mwRqoSMo5T728TH7p8vRKsd0XY73O8/PHZI8TND6acYyX+tp/qJibgMX67SEnixworDrPAEYaxseDs9i6N+zcVEMqNIHALWFJGV613iZ9DzCbmHs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734644218; c=relaxed/simple; bh=3S23dTTP7NsKmXAV0bzH89sOImqZmkUX/E13JUSWaW0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=YF4fMtuUqC4jI2Emz0IA2Jl9Dik3F43ExJcjmSsWLVL+lhbv2TpMBexO0hmp6I016uK+jPPpgXC3aFSagrsivf2s0WlWMs/SPITWviqzmw3s8JnVjJVONELW8Ss2z2n325alFty63/57pNTlN7s9CGzBPXeGuE4R6bphwFGAOiU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com; spf=pass smtp.mailfrom=rivosinc.com; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b=sfm9t730; arc=none smtp.client-ip=209.85.214.179 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b="sfm9t730" Received: by mail-pl1-f179.google.com with SMTP id d9443c01a7336-2164b1f05caso12328005ad.3 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=vger.kernel.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=sfm9t730EGhHY/HDvwVTSzZ6n+w4e5zbkvbO3U3TktsIyyvsMuC1DpteOC9joE4eye UnX8mxwqaI6JtFfHp/dnCfOBJLKfWYxC3+J+0elcFDji9ACm7T/gPL64zkoeqZ8dWEKd us4a/6RiS1c9wHvm5fzGE/L6lX7pzDO5lY22OduTQ8iy0VFMP3SB1GmTGQgTAVnOVoY+ h9XXQriSE3oBbfHwrxctU71DmNQLt+hWD559NBo6gumI1/+w0m1AaS1+Pzq2OToYAoer q2b0yFOY+8cgu4isaFK0r8ObbeMvTEeT0my2zmbBfCqqK8HcNnegpqwE/nCQOfedkno+ 3kZA== 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=ZPjvF0oZdkUG+HxaB3a6ntvPfwHA08viJEyGYqai2OAHkZCCDVBdfvQUBhCns6vtE6 Y3ORnLtwRTaECOee+2a6tc6x+XegFPL7CRoCom/BQkaZAoyrZjs8e1sjjPV7/SNRUY5N VKdp0OUzcUSZBfPyBemWJCoqw3+qVKWI7qj1Klkfy5xw6e7vQb4zvohsiIjHV40VmFHW y5ltZX0/1K0nHzXQwJlIoYjqWz+wYrreuaGKYWj5s+4yLoJiIvxtCsBa48r6CMpjqO3t dZJYfJAb3UcXseQOJw9FkDExzwDqyye5/ZHIJBnUHwdH9gP6iytsh7CrA2ho1FLQM6Tq Fb4w== X-Forwarded-Encrypted: i=1; AJvYcCU8pENwKU2VyTHwjcZkxLI36gHaBtUTSOV9Nl41ZSEJrYBhhi1BOmIqy4O3aaTiaOQAHpafnaI=@vger.kernel.org X-Gm-Message-State: AOJu0YznYpMg/N3KH7iQXCWgBhET2RSU5FY62pfFfHu0qwwJvueGMQq3 By/ww/O42K4YKsAtKamqbSmj/BDQxCCmYtbT/nK/sjjRD/ezuvoZLaTicsuySTo= X-Gm-Gg: ASbGnctj2QsglGqfmPElRPq7NRg1EUT4SUYYt1bo8VwA2ApxCTwVtCgkqm1pdr+j8qb TPWyaL367fRXcrWPWLySnZ7zbxAYMWNWP68dYMr8SRD3lij0TUsPtEI9rFeWl0ahdeFdNrqMtDO QpJBPaJ+VFHly0WRtToTR4wi85oiaujFvHKJj84qgNcICAKsyxacsnMqwtSgUWr8tNmzAXr7c6a 5ls925/INtz96njNIwhu+o8c0wLd416RMED+5euODNyoAg= 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> Precedence: bulk X-Mailing-List: stable@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50a62291-5ae1-47b0-8f64-a42271820789@coelacanthus.name> 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 >