linux-kernel - Re: [PATCH v1 2/2] kselftest: vm: add tests for memory-deny-write-execute

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Nov 2022 17:33:03 +0000
From:   Joey Gouly <joey.gouly@....com>
To:     Mark Brown <broonie@...nel.org>
CC:     Catalin Marinas <catalin.marinas@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Lennart Poettering <lennart@...ttering.net>,
        Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>,
        "Alexander Viro" <viro@...iv.linux.org.uk>,
        Kees Cook <keescook@...omium.org>,
        "Szabolcs Nagy" <szabolcs.nagy@....com>,
        Jeremy Linton <jeremy.linton@....com>,
        "Topi Miettinen" <toiwoton@...il.com>, <linux-mm@...ck.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-abi-devel@...ts.sourceforge.net>, <nd@....com>,
        <shuah@...nel.org>
Subject: Re: [PATCH v1 2/2] kselftest: vm: add tests for
 memory-deny-write-execute

Hi,

On Fri, Oct 28, 2022 at 06:03:18PM +0100, Mark Brown wrote:
> On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote:
> 
> > Add some tests to cover the new PR_SET_MDWE prctl.
> 
> Some comments below but they're all stylistic and let's not make perfect
> be the enemy of the good here so
> 
> Reviewed-by: Mark Brown <broonie@...nel.org>

Thanks for the review, however I won't keep your R-b tag because I'm going to
move forward with Kees' approach from:

https://lore.kernel.org/linux-arm-kernel/202210281314.C5D3414722@keescook/T/#m45ac9de6c205b560d072a65e4e67e2a7ee363588

Thanks to Kees for rewriting that.

> 
> and we can iterate later rather than blocking anything on the testcase.
> 
> > +#ifdef __aarch64__
> > +#define PROT_BTI      0x10            /* BTI guarded page */
> > +#endif
> 
> We should get this from the kernel headers shouldn't we?  We generally
> rely on things getting pulled in from there rather than locally
> defining.

I believe the mman.h included is from the toolchain, not the kernel's uapi headers.
The toolchain I was using didn't have PROT_BTI defined in its mman.h

> 
> > +#define TEST1 "mmap(PROT_WRITE | PROT_EXEC)\n"
> > +#define TEST2 "mmap(PROT_WRITE); mprotect(PROT_EXEC)\n"
> > +#define TEST3 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_READ)\n"
> > +#define TEST4 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_BTI)\n"
> 
> > +int test1(int mdwe_enabled)
> > +{
> 
> It feels like we could usefully make an array of
> 
> 	struct test {
> 		int (*run)(bool mdwe_enabled);
> 		char *name;
> 	}
> 
> then we'd need fewer ifdefs, things could be more usefully named and
> it'd be a bit easier to add new cases.
> 
> > +#ifdef __aarch64__
> > +	ksft_set_plan(12);
> > +#else
> > +	ksft_set_plan(9);
> > +#endif
> 
> That'd just be ksft_test_plan(3 * ARRAY_SIZE(tests).
> 
> > +	// First run the tests without MDWE
> > +	test_result(test1(0), TEST1);
> > +	test_result(test2(0), TEST2);
> > +	test_result(test3(0), TEST3);
> > +#ifdef __aarch64__
> > +	test_result(test4(0), TEST4);
> > +#endif
> 
> and these calls to the tests would all be iterating over the array.

These comments are solved by the kselftest_harness approach that Kees suggested.

Thanks,
Joey