[cyphar]

These days it'd be harder to write code which is "easy to overlook" -- the innocent version would be something like

  if (/* ... */ || current_euid() == GLOBAL_ROOT_KUID)

But the "backdoor" version would fail to compile (current_euid() is a macro but it's written to not be a permitted lvalue). You would need to write something more obvious like the following (and kernel devs would go "huh?" upon seeing the usage of current_cred() in this context)

  if (/* ... */ || current_cred()->euid = GLOBAL_ROOT_KUID)

In addition, comparisons against UIDs directly are no longer as common because of user namespaces and capabilities -- correct code would be expected to look more like

  if (/* ... */ || capable(CAP_SYS_ADMIN))

Which you can't write as an "accidental" exploit. And since most permission checks these days use capabilities rather than raw UIDs you'd need to do

  commit_creds(get_cred(&init_cred));

Which is bound to raise more than a couple of eyebrows and is really non-trivial to hide (assuming you put it somewhere as obvious as this person did).

But I will say that it would've been much more clever to hide it in a device driver which is widely included as a built-in in distribution kernels. I imagine if you managed to compromise Linus' machine (and there are ways of "hiding" changes in merge commits) then the best place would be to shove the change somewhere innocuous like the proc connector (which is reachable via unprivileged netlink, is enabled on most distribution kernels, and is not actively maintained so nobody will scream about it). But these days we also have bots which actively scan people's trees and try to find exploits (including the 0day project and syzkaller), so such obvious bugs probably would still be caught.

[perihelions]

"(current_euid() is a macro but it's written to not be a permitted lvalue)"

I'm not an expert at C. I followed up on this kernel macro out of curiosity, and it was a confusing learning experience because it turns out the forbidden assignment

    ({ x; }) = y;

is silently permitted by GCC (for example, with -Wall --std={c99,c11,c18}), and does actually assign x=y. Even though that's expressly prohibited by the C standard (-Wpedantic).

I assume this is old news to C programmers, but its insidiousness surprised me.

[simias]

Example #5984 of why I don't like the kernel's convention of masquerading macros as function by making them lowercase. I wasted so much time deciphering weird compile errors or strange behaviour only to finally realize that one of the function calls in the offending code was actually a macro in disguise.

It's especially bad when some kernel macros, such as wait_event, don't even behave like a function would (evaluating the parameter repeatedly).

One more thing Rust got right by suffixing macros with a mandatory !.

[cyphar]

The best one is "current" -- which is a macro that looks like a variable but becomes a function call and thus if you ever want to use the variable name "current" you will get build errors. :D

[cyphar]

Huh, I assumed (just as you did) that this would obviously not work -- but you're right that GCC ignores this and allows the assignment anyway.

However it turns out that you still get a build error, and even the more explicit versions also give you a error:

  kernel/cred.c:763:17: error: assignment of member ‘euid’ in read-only object
    763 |  current_euid() = GLOBAL_ROOT_UID;
        |                 ^
  kernel/cred.c:764:23: error: assignment of member ‘euid’ in read-only object
    764 |  current_cred()->euid = GLOBAL_ROOT_UID;
        |                       ^
  kernel/cred.c:765:22: error: assignment of member ‘euid’ in read-only object
    765 |  current->cred->euid = GLOBAL_ROOT_UID;
        |                      ^

So it is blocked but not for the reason I thought. current_cred() returns a const pointer and all of the cred pointers in task_struct are also const. So you'd need to do something more like:

  ((struct cred *)current_cred())->euid = GLOBAL_ROOT_UID;

Which is well beyond "eyebrow-raising" territory.

[pwdisswordfish4]

The C standard cannot expressly prohibit anything about a feature that isn't part of the C standard.

[kazinator]

What you quoted:

    ({ x; }) = y;

isn't ISO C syntax; it's a GNU extension.

-Wpedantic diagnoses ISO C syntax errors, even if they are GNU extensions.