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 A006EC021A1 for ; Tue, 11 Feb 2025 08:59:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 040416B0088; Tue, 11 Feb 2025 03:59:07 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F3CE16B0089; Tue, 11 Feb 2025 03:59:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E2168280001; Tue, 11 Feb 2025 03:59:06 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id C4D446B0088 for ; Tue, 11 Feb 2025 03:59:06 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 60B7EA1422 for ; Tue, 11 Feb 2025 08:58:37 +0000 (UTC) X-FDA: 83107063116.12.E460289 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf08.hostedemail.com (Postfix) with ESMTP id 606D116000B for ; Tue, 11 Feb 2025 08:58:35 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf08.hostedemail.com: domain of kevin.brodsky@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=kevin.brodsky@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1739264315; a=rsa-sha256; cv=none; b=O5w8xkT2+PkuPGVZ5CfwKumtl0mKUC0Wf3CBCjghn+mNxUG0J9of8xARsu60p2LI0sFdID nn0gUIuXHJfNVcgkNjEgLi+oxx3TbfkcjXMgJdceh2V7biMez6xqhr7vR3IC/W4Y2p81U6 MDFEffvI7840/26EA3b9WcogPD2ZPGY= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf08.hostedemail.com: domain of kevin.brodsky@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=kevin.brodsky@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1739264315; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wQh+YpaHfdrK1kjIhTBoMDrV84DSrT9CIwd9K0b6WSE=; b=VJmLo9ePDJK+ual7DjLencCW/W4Dk1BvPSzIQsqRGrAucAxCLAKAmZ4GntAheYwg1RO4rC mSxA5nJ1IXrPSNNOBjSXFcyEZgcfsQ5jgYu4kNA+1SYcfkWyxxN1fkB4YjIIZRk/sFVjYa P42hn/fYNgFqs0ZtMIWHpMcy5Oin3EA= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C72091477; Tue, 11 Feb 2025 00:58:55 -0800 (PST) Received: from [10.57.83.200] (unknown [10.57.83.200]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9FBD13F5A1; Tue, 11 Feb 2025 00:58:28 -0800 (PST) Message-ID: Date: Tue, 11 Feb 2025 09:58:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH 8/8] mm: Add basic tests for kpkeys_hardened_cred To: Kees Cook Cc: linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org, Andrew Morton , Mark Brown , Catalin Marinas , Dave Hansen , David Howells , "Eric W. Biederman" , Jann Horn , Jeff Xu , Joey Gouly , Linus Walleij , Andy Lutomirski , Marc Zyngier , Peter Zijlstra , Pierre Langlois , Quentin Perret , "Mike Rapoport (IBM)" , Ryan Roberts , Thomas Gleixner , Will Deacon , Matthew Wilcox , Qi Zheng , linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, x86@kernel.org References: <20250203102809.1223255-1-kevin.brodsky@arm.com> <20250203102809.1223255-9-kevin.brodsky@arm.com> <202502062024.BCB0DED1D5@keescook> Content-Language: en-GB From: Kevin Brodsky In-Reply-To: <202502062024.BCB0DED1D5@keescook> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 606D116000B X-Stat-Signature: w3jriuzx1xpz8s4euh6i8gi7ji7w5q9m X-Rspam-User: X-HE-Tag: 1739264315-406688 X-HE-Meta: U2FsdGVkX1+7SE//7MtSStzIgx8zat4cbAt+FnnQrxI9lWLfbYJXAO4d956k66sYQeZBmwyFcPnXII0mn5igQuZbfcNUDPKJMHz/lMu0mPD3HMd4Uo9IG8R/dHBgAKn3CcfTzhJ1ctD7XRNR4T99+ukzAvAqjRdu2mNWhLmoLWE3f43JckgDlJuxZ8qqyoi45ANUocxHkBZoUbZJuNyHsNgHRySeCeEMQLof1bcu+6ZnuzgGJhqvNOTcPP084tcQxjO4YGC+pjnRgsd8v1ptpWGrdSfgDWoNDuHcLUUC3UKuJqe3o/47n0QfxwnKQNDV/JnQj5wL4V/nzJcFHW8rCc9+fo9TjSf1Sn5t15NbI52vzY3d4DJ3nzvUgiFx8siJwZ7P0gQIb7uAKqCWvW5s8UNUZ50hT3k9VqtkhenAE1ETGzKcGPO9XCOZ79WAZLhAwwgQBjXegoWYMtG9Ujzf5F1ka582VxPSG0uSVwoPxXu+G+FadA0ety+twO0KYHS4EEmOeshaLPYKE55y+ERzeSSEek7RFglbvoc/i1Q8uuBVT+W14R/O/H2yxSlicmJJR9jZaUpyDNYjTwmeZIkc54w950o6GYtL5sI9CYUH0X2hx/Qr6snqzC9/pfXcCVT3h4ftJm2igm2DzAVJTowz2HwycWAnLoFr/nL1o4w1HPT56b6Dn6+EA1U4/dKH3MD8MZjiF96gGdNqZnFovfNElm6GBdTEBP07PniMNqrgY7xA6dBkhRP45PxkWYRmGiSbiz7FIffxaEH8X0geypBXS5BxU6dPvKJb4pkus+LlXXnxOfwsiw/Ze7n2AsbvffN030Z9p/4CzYZMDBXqQfTVTVMx92+3shPhrxk12IzFt9eVy5a3ie460OPxuZdLV+sutvs8q6irLbu8X+2wl4NwT3+VSvbjT6vS3O5ttH0KDSJRIR3ZTcMxolre6/17kkWTuYIrAj1PmsFWp1aqpGl fsW1WDdE 812X5FHx7NBS5DTkrnWxN4mdocHzSJhfXXa7mTt46flMZrIfXeg5QG4jTsexMBK8G+vgbj/rAzRmLbaI7bS7LiGyCzrn365erroJryU9DOvbS40rYutiV3HR3wIOJcicqpHzRKTCPw3hjK4omAEbVnxjbiujy50DS2W/4KyPV9VTBeMCnsnlKon9KsBRy4k1rhvFnCS6CcCan1gUJ9oKfo2sAsBoy++kHcLQPHk/W5+6q6Fvw3GfXxkTWvw== 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: List-Subscribe: List-Unsubscribe: On 07/02/2025 05:52, Kees Cook wrote: > On Mon, Feb 03, 2025 at 10:28:09AM +0000, Kevin Brodsky wrote: >> Add basic tests for the kpkeys_hardened_pgtables feature: try to >> perform a direct write to current->{cred,real_cred} and ensure it >> fails. >> >> Signed-off-by: Kevin Brodsky >> --- >> mm/Makefile | 1 + >> mm/kpkeys_hardened_cred_test.c | 42 ++++++++++++++++++++++++++++++++++ > Current file naming convention[1] would be to name this as: > > mm/tests/kpkeys_hardened_cred_kunit.c I wasn't aware of those guidelines, thanks for the pointer! I got inspiration from various existing tests, it unfortunately looks like the conventions in [1] have not been universally adopted. I'll try to follow them in the next version (of both RFC series). > [...] > > +static void write_cred(struct kunit *test) > +{ > + long zero = 0; > + int ret; > + > + ret = copy_to_kernel_nofault((unsigned long *)current->cred, &zero, sizeof(zero)); > + KUNIT_EXPECT_EQ_MSG(test, ret, -EFAULT, > + "Write to current->cred wasn't prevented"); > + > + ret = copy_to_kernel_nofault((unsigned long *)current->real_cred, &zero, sizeof(zero)); > + KUNIT_EXPECT_EQ_MSG(test, ret, -EFAULT, > + "Write to current->real_cred wasn't prevented"); > This is a good negative test. I would include a positive test as well. > i.e. make sure you can run copy_from_kernel_nofault() to read it > successfully. Otherwise you don't know if you're just getting a bad > address -- we want to distinguish between them. (This is more true for > the next suggestion, since current->cred being broken would be much more > obvious.) That's a fair point, I've actually run into this sort of issues with the page table tests (in the other RFC series). I can add positive tests with a regular read (e.g. reading current->cred->uid directly) - no fault is expected to occur in that case. > While current->cred is good and easy, I would like to see prepare_creds() > exercised too to get a new cred and validate that it is equally directly > readable and directly not writable, and then use the correct accessors > to perform a successful write to the cred, read back the change, > etc. (i.e. validate the expected behavior too.) prepare_creds() does not allocate protected memory, see the introduction in the cover letter and patch 6. However I could certainly add such tests for the new helpers protect_creds() and prepare_protected_creds(), which are meant to be used with override_creds(). >> +} >> + >> +static int kpkeys_hardened_cred_suite_init(struct kunit_suite *suite) >> +{ >> + if (!arch_kpkeys_enabled()) { >> + pr_err("Cannot run kpkeys_hardened_cred tests: kpkeys are not supported\n"); >> + return 1; >> + } > Instead of failing ("return 1") I think this should be a "skip" (it is > expected to not work if there is no support) in each test instead: kasan_suite_init() uses this approach if KASAN is disabled, but skipping does seem to be a better idea - this way it doesn't show up as an error. > if (!arch_kpkeys_enabled()) > kunit_skip(test, "kpkeys are not supported\n"); > > I'm very happy to see tests! :) Thank you for the review and suggestions! - Kevin