Re: [PATCH v2 3/5] mm: Make PR_MDWE_REFUSE_EXEC_GAIN an unsigned long

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22.05.23 12:35, Alexey Izbyshev wrote:
On 2023-05-22 11:55, David Hildenbrand wrote:
On 17.05.23 17:03, Florent Revest wrote:
Alexey pointed out that defining a prctl flag as an int is a footgun
because, under some circumstances, when used as a flag to prctl, it
can
be casted to long with garbage upper bits which would result in
unexpected behaviors.

This patch changes the constant to a UL to eliminate these
possibilities.

Signed-off-by: Florent Revest <revest@xxxxxxxxxxxx>
Suggested-by: Alexey Izbyshev <izbyshev@xxxxxxxxx>
---
   include/uapi/linux/prctl.h       | 2 +-
   tools/include/uapi/linux/prctl.h | 2 +-
   2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index f23d9a16507f..6e9af6cbc950 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -283,7 +283,7 @@ struct prctl_mm_map {
     /* Memory deny write / execute */
   #define PR_SET_MDWE			65
-# define PR_MDWE_REFUSE_EXEC_GAIN	1
+# define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
     #define PR_GET_MDWE			66
   diff --git a/tools/include/uapi/linux/prctl.h
b/tools/include/uapi/linux/prctl.h
index 759b3f53e53f..6e6563e97fef 100644
--- a/tools/include/uapi/linux/prctl.h
+++ b/tools/include/uapi/linux/prctl.h
@@ -283,7 +283,7 @@ struct prctl_mm_map {
     /* Memory deny write / execute */
   #define PR_SET_MDWE			65
-# define PR_MDWE_REFUSE_EXEC_GAIN	1
+# define PR_MDWE_REFUSE_EXEC_GAIN	(1UL << 0)
     #define PR_GET_MDWE			66


Both are changing existing uapi, so you'll already have existing user
space using the old values, that your kernel code has to deal with no?

I'm the one who suggested this change, so I feel the need to clarify.

For any existing 64-bit user space code using the kernel and the uapi
headers before this patch and doing the wrong prctl(PR_SET_MDWE,
PR_MDWE_REFUSE_EXEC_GAIN) call instead of the correct prctl(PR_SET_MDWE,
(unsigned long)PR_MDWE_REFUSE_EXEC_GAIN), there are two possibilities
when prctl() implementation extracts the second argument via va_arg(op,
unsigned long):

* It gets lucky, and the upper 32 bits of the argument are zero. The
call does what is expected by the user.

* The upper 32 bits are non-zero junk. The flags argument is rejected by
the kernel, and the call fails with EINVAL (unexpectedly for the user).

This change is intended to affect only the second case, and only after
the program is recompiled with the new uapi headers. The currently
wrong, but naturally-looking prctl(PR_SET_MDWE,
PR_MDWE_REFUSE_EXEC_GAIN) call becomes correct.

The kernel ABI is unaffected by this change, since it has been defined
in terms of unsigned long from the start.

The thing I'm concerned about is the following: old user space (that would fail) on new kernel where we define some upper 32bit to actually have a meaning (where it would succeed with wrong semantics).

IOW, can we ever really "use" these upper 32bit, or should we instead only consume the lower 32bit in the kernel and effectively ignore the upper 32bit?

I guess the feature is not that old, so having many existing user space applications is unlikely.

Which raises the question if we want to tag this here with a "Fixes" and eventually cc stable (hmm ...)?

--
Thanks,

David / dhildenb





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux