Re: [PATCH 4/4] kselftest: vm: Add tests for no-inherit memory-deny-write-execute

[Florent Revest]

On Fri, May 5, 2023 at 11:26 PM Alexey Izbyshev <izbyshev@xxxxxxxxx> wrote:
>
> On 2023-05-05 19:42, Florent Revest wrote:
> > On Thu, May 4, 2023 at 10:30 PM Alexey Izbyshev <izbyshev@xxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> >> > ---
> >> >  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