Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Thu, 04 May 2023 23:29:58 +0300
From:   Alexey Izbyshev <izbyshev@ispras.ru>
To:     Florent Revest <revest@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, catalin.marinas@arm.com,
        anshuman.khandual@arm.com, joey.gouly@arm.com, mhocko@suse.com,
        keescook@chromium.org, david@redhat.com, peterx@redhat.com,
        nd@arm.com, broonie@kernel.org, szabolcs.nagy@arm.com
Subject: Re: [PATCH 4/4] kselftest: vm: Add tests for no-inherit
 memory-deny-write-execute
In-Reply-To: <20230504170942.822147-5-revest@chromium.org>
References: <20230504170942.822147-1-revest@chromium.org>
 <20230504170942.822147-5-revest@chromium.org>
User-Agent: Roundcube Webmail/1.4.4
Message-ID: <66900d0ad42797a55259061f757beece@ispras.ru>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023-05-04 20:09, Florent Revest wrote:
> Add some tests to cover the new PR_MDWE_NO_INHERIT flag of the
> PR_SET_MDWE prctl.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  tools/testing/selftests/mm/mdwe_test.c | 95 ++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/mdwe_test.c
> b/tools/testing/selftests/mm/mdwe_test.c
> index 91aa9c3099e7..9f08ed1b99ae 100644
> --- a/tools/testing/selftests/mm/mdwe_test.c
> +++ b/tools/testing/selftests/mm/mdwe_test.c
> @@ -22,6 +22,8 @@
> 
>  TEST(prctl_flags)
>  {
> +	EXPECT_LT(prctl(PR_SET_MDWE, PR_MDWE_NO_INHERIT, 0L, 0L, 7L), 0);
> +

PR_MDWE_NO_INHERIT is defined to an int constant, so passing it to 
prctl() without a cast to long or similar may produce wrong code on 
64-bit targets (ABIs typically don't require the compiler to clear the 
upper 32 bits of a 64-bit register when passing a 32-bit argument, so 
va_arg(arg, unsigned long) in prctl() implementation might get junk).

Arguably, defining PR_MDWE_* to plain int constants is a bug, or at 
least a footgun for users of uapi headers.

Thanks,
Alexey

>  	EXPECT_LT(prctl(PR_SET_MDWE, 7L, 0L, 0L, 0L), 0);
>  	EXPECT_LT(prctl(PR_SET_MDWE, 0L, 7L, 0L, 0L), 0);
>  	EXPECT_LT(prctl(PR_SET_MDWE, 0L, 0L, 7L, 0L), 0);
> @@ -33,6 +35,66 @@ TEST(prctl_flags)
>  	EXPECT_LT(prctl(PR_GET_MDWE, 0L, 0L, 0L, 7L), 0);
>  }
> 
> +FIXTURE(consecutive_prctl_flags) {};
> +FIXTURE_SETUP(consecutive_prctl_flags) {}
> +FIXTURE_TEARDOWN(consecutive_prctl_flags) {}
> +
> +FIXTURE_VARIANT(consecutive_prctl_flags)
> +{
> +	unsigned long first_flags;
> +	unsigned long second_flags;
> +	bool should_work;
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, same)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.should_work = true,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_disable_mdwe)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.second_flags = 0,
> +	.should_work = false,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, 
> cant_disable_mdwe_no_inherit)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> +	.second_flags = 0,
> +	.should_work = false,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, can_lower_privileges)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> +	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.should_work = true,
> +};
> +
> +FIXTURE_VARIANT_ADD(consecutive_prctl_flags, cant_gain_privileges)
> +{
> +	.first_flags = PR_MDWE_REFUSE_EXEC_GAIN,
> +	.second_flags = PR_MDWE_REFUSE_EXEC_GAIN | PR_MDWE_NO_INHERIT,
> +	.should_work = false,
> +};
> +
> +TEST_F(consecutive_prctl_flags, two_prctls)
> +{
> +	int ret;
> +
> +	EXPECT_EQ(prctl(PR_SET_MDWE, variant->first_flags, 0L, 0L, 0L), 0);
> +
> +	ret = prctl(PR_SET_MDWE, variant->second_flags, 0L, 0L, 0L);
> +	if (variant->should_work) {
> +		EXPECT_EQ(ret, 0);
> +	} else {
> +		EXPECT_NE(ret, 0);
> +	}
> +}
> +
>  FIXTURE(mdwe)
>  {
>  	void *p;
> @@ -45,28 +107,45 @@ FIXTURE_VARIANT(mdwe)
>  {
>  	bool enabled;
>  	bool forked;
> +	bool inherit;
>  };
> 
>  FIXTURE_VARIANT_ADD(mdwe, stock)
>  {
>  	.enabled = false,
>  	.forked = false,
> +	.inherit = false,
>  };
> 
>  FIXTURE_VARIANT_ADD(mdwe, enabled)
>  {
>  	.enabled = true,
>  	.forked = false,
> +	.inherit = true,
>  };
> 
> -FIXTURE_VARIANT_ADD(mdwe, forked)
> +FIXTURE_VARIANT_ADD(mdwe, inherited)
>  {
>  	.enabled = true,
>  	.forked = true,
> +	.inherit = true,
>  };
> 
> +FIXTURE_VARIANT_ADD(mdwe, not_inherited)
> +{
> +	.enabled = true,
> +	.forked = true,
> +	.inherit = false,
> +};
> +
> +static bool executable_map_should_fail(const FIXTURE_VARIANT(mdwe) 
> *variant)
> +{
> +	return variant->enabled && (!variant->forked || variant->inherit);
> +}
> +
>  FIXTURE_SETUP(mdwe)
>  {
> +	unsigned long mdwe_flags;
>  	int ret, status;
> 
>  	self->p = NULL;
> @@ -76,13 +155,17 @@ FIXTURE_SETUP(mdwe)
>  	if (!variant->enabled)
>  		return;
> 
> -	ret = prctl(PR_SET_MDWE, PR_MDWE_REFUSE_EXEC_GAIN, 0L, 0L, 0L);
> +	mdwe_flags = PR_MDWE_REFUSE_EXEC_GAIN;
> +	if (!variant->inherit)
> +		mdwe_flags |= PR_MDWE_NO_INHERIT;
> +
> +	ret = prctl(PR_SET_MDWE, mdwe_flags, 0L, 0L, 0L);
>  	ASSERT_EQ(ret, 0) {
>  		TH_LOG("PR_SET_MDWE failed or unsupported");
>  	}
> 
>  	ret = prctl(PR_GET_MDWE, 0L, 0L, 0L, 0L);
> -	ASSERT_EQ(ret, 1);
> +	ASSERT_EQ(ret, mdwe_flags);
> 
>  	if (variant->forked) {
>  		self->pid = fork();
> @@ -113,7 +196,7 @@ TEST_F(mdwe, mmap_READ_EXEC)
>  TEST_F(mdwe, mmap_WRITE_EXEC)
>  {
>  	self->p = mmap(NULL, self->size, PROT_WRITE | PROT_EXEC, self->flags, 
> 0, 0);
> -	if (variant->enabled) {
> +	if (executable_map_should_fail(variant)) {
>  		EXPECT_EQ(self->p, MAP_FAILED);
>  	} else {
>  		EXPECT_NE(self->p, MAP_FAILED);
> @@ -139,7 +222,7 @@ TEST_F(mdwe, mprotect_add_EXEC)
>  	ASSERT_NE(self->p, MAP_FAILED);
> 
>  	ret = mprotect(self->p, self->size, PROT_READ | PROT_EXEC);
> -	if (variant->enabled) {
> +	if (executable_map_should_fail(variant)) {
>  		EXPECT_LT(ret, 0);
>  	} else {
>  		EXPECT_EQ(ret, 0);
> @@ -154,7 +237,7 @@ TEST_F(mdwe, mprotect_WRITE_EXEC)
>  	ASSERT_NE(self->p, MAP_FAILED);
> 
>  	ret = mprotect(self->p, self->size, PROT_WRITE | PROT_EXEC);
> -	if (variant->enabled) {
> +	if (executable_map_should_fail(variant)) {
>  		EXPECT_LT(ret, 0);
>  	} else {
>  		EXPECT_EQ(ret, 0);