[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <66900d0ad42797a55259061f757beece@ispras.ru>
Date: Thu, 04 May 2023 23:29:58 +0300
From: Alexey Izbyshev <izbyshev@...ras.ru>
To: Florent Revest <revest@...omium.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
akpm@...ux-foundation.org, catalin.marinas@....com,
anshuman.khandual@....com, joey.gouly@....com, mhocko@...e.com,
keescook@...omium.org, david@...hat.com, peterx@...hat.com,
nd@....com, broonie@...nel.org, szabolcs.nagy@....com
Subject: Re: [PATCH 4/4] kselftest: vm: Add tests for no-inherit
memory-deny-write-execute
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@...omium.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);
Powered by blists - more mailing lists