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
References: <20230504170942.822147-1-revest@chromium.org> <20230504170942.822147-5-revest@chromium.org>
 <66900d0ad42797a55259061f757beece@ispras.ru> <CABRcYmKXrPE0oP9R=qsn=mL3Nwm_mVqT-r-TvOvowTdiuYU--A@mail.gmail.com>
 <3a38319a3b241e578729ffa5484ad24b@ispras.ru>
In-Reply-To: <3a38319a3b241e578729ffa5484ad24b@ispras.ru>
From:   Florent Revest <revest@chromium.org>
Date:   Mon, 8 May 2023 14:12:14 +0200
Message-ID: <CABRcYm+SUV9HThByGcTfS8K+f5KRxgD1SWnc5iqWTqgipsCP0g@mail.gmail.com>
Subject: Re: [PATCH 4/4] kselftest: vm: Add tests for no-inherit memory-deny-write-execute
To:     Alexey Izbyshev <izbyshev@ispras.ru>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 5, 2023 at 11:26=E2=80=AFPM Alexey Izbyshev <izbyshev@ispras.ru=
> wrote:
>
> On 2023-05-05 19:42, Florent Revest wrote:
> > On Thu, May 4, 2023 at 10:30=E2=80=AFPM Alexey Izbyshev <izbyshev@ispra=
s.ru>
> > wrote:
> >>
> >> 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).
> >
> > Ah, good catch Alexey! :)
> >
> >> Arguably, defining PR_MDWE_* to plain int constants is a bug, or at
> >> least a footgun for users of uapi headers.
> >
> > As part of the next version of this series, I'm happy to:
> > 1- change the existing PR_MDWE_REFUSE_EXEC_GAIN to 1UL
> > 2- introduce PR_MDWE_NO_INHERIT as 2UL
> >
> Yes, I think it's the right thing to do. I suggest to spell them as (1UL
> << 0), etc. for consistency with all other unsigned long flags in the
> header.

Ah yeah, absolutely! Good tip too, thank you :)

> > But I'm surprised that most of the macros in
> > include/uapi/linux/prctl.h are the same sort of footguns already ?
> > Hasn't it been an issue for other prctls yet ?
>
> Yes, they are. I'm not aware of a public discussion of this specific
> issue, but note that at least for some prctl() options the kernel
> doesn't care about upper bits because arguments are truncated before
> doing anything else with them (e.g. for PR_SCHED_CORE raw prctl()

That makes sense

> arguments are implicitly converted to what sched_core_share_pid()
> expects). Also, actually getting junk in the upper bits might not always
> be easy (e.g. on x86-64 all or almost all instructions with r32
> destination operand clear the upper bits). Unfortunately, I don't have a
> better answer than this.

Okay, I was just curious, that's good to know