Wouldn't that also suffer from the same issue, or how is this
different?
Yes, it is the same issue, so e.g. prctl(PR_SET_DUMPABLE,
SUID_DUMP_DISABLE ) may wrongly fail with EINVAL on 64-bit targets.
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)
Yes, this is not reliable on 64-bit targets too. The simplest fix is to
use "0L", as done in MDWE self-tests (but many other tests get this
wrong).
Oh, it's even worse than I thought, then. :)
Even in our selftest most of
$ git grep prctl tools/testing/selftests/ | grep "0"
gets it wrong.
Florent also expressed surprise[1] that we don't see a lot of failures
due to such issues, and I tried to provide some reasons. To elaborate on
Yes, I'm also surprised!
the x86-64 thing, for prctl(PR_SET_DUMPABLE, 0) the compiler will likely
generate "xorl %esi, %esi" to pass zero, but this instruction will also
clear the upper 32 bits of %rsi, so the problem is masked (and I believe
CPU vendors are motivated to do such zeroing to reduce false
dependencies). But this zeroing is not required by the ABI, so in a more
complex situation junk might get through.
:/
Real-world examples of very similar breakage in variadic functions
involving NULL sentinels are mentioned in [2] (the musl bug report is
[3]). In short, musl defined NULL as plain 0 for C++, so when people do
e.g. execl("/bin/true", "true", NULL), junk might prevent detection of
the sentinel in execl() impl. (Though if the sentinel is passed via
stack because there are a lot of preceding arguments, the breakage
becomes more apparent because auto-zeroing of registers doesn't come
into play anymore.)
Yes, I heard about the "fun" with NULL already. Thanks for the musl
pointer. And thanks for the confirmation/explanation.
I'm easily confused by such (va_args) things, so sorry for the dummy
questions.
This stuff *is* confusing, and note that Linux man pages don't even tell
that prctl() is actually declared as a variadic function (and for
ptrace() this is mentioned only in the notes, but not in its signature).
Agreed, that's easy to miss (and probably many people missed it).
Anyhow, for this patch as is (although it feels like drops in the ocean
after our discussion)
Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>
--
Thanks,
David / dhildenb
