Messages in this thread | | | Date | Tue, 23 May 2023 11:12:37 +0200 | From | David Hildenbrand <> | Subject | Re: [PATCH v2 3/5] mm: Make PR_MDWE_REFUSE_EXEC_GAIN an unsigned long |
| |
On 22.05.23 20:58, Alexey Izbyshev wrote: > On 2023-05-22 19:22, David Hildenbrand wrote: >> 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@chromium.org> >>>>> Suggested-by: Alexey Izbyshev <izbyshev@ispras.ru> >>>>> --- >>>>> 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 see, thanks. But I think this question is mostly independent from this > patch. The patch removes a footgun, but it doesn't change the flags > check in the kernel, and nothing stops the user from doing > > int flags = PR_MDWE_REFUSE_EXEC_GAIN; > prctl(PR_SET_MDWE, flags); > > So we have to decide whether to ignore the upper 32 bits or not even if > this patch is not applied (actually *had to* when PR_SET_MDWE op was > being added).
Well, an alternative to this patch would be to say "well, for this prctl we ignore any upper 32bit. Then, this change would not be needed. Yes, I also don't like that :)
Bu unrelated, I looked at some other random prctl.
#define SUID_DUMP_USER 1
And in kernel/sys.c:
case PR_SET_DUMPABLE: if (arg2 != SUID_DUMP_DISABLE && arg2 != SUID_DUMP_USER) ...
Wouldn't that also suffer from the same issue, or how is this different?
Also, how is passing "0"s to e.g., PR_GET_THP_DISABLE reliable? We need arg2 -> arg5 to be 0. But wouldn't the following also just pass a 0 "int" ?
prctl(PR_GET_THP_DISABLE, 0, 0, 0, 0)
I'm easily confused by such (va_args) things, so sorry for the dummy questions.
-- Thanks,
David / dhildenb
| |