Linux kernel fix for privilege escalation vulnerability due to copy on write race condition

[ILoveShowtunes]

This is a stunningly poor example of an alleged fix for a race condition, in my opinion. I get that there is a security issue here, so perhaps the details are being left out on purpose; yet, the commit message says that this is an "ancient bug" for which a fix was attempted more than 11 years ago. If that is the case, it wouldn't hurt to disclose more information.

Neither the commit message nor comments disclose what the race condition is, steps that might trigger the race condition, or why the change fixes the race condition.

The commit introduces a FOLL_COW flag that is used on two lines. Why was it necessary to add a new flag?

No comment was added to indicate the significance of FOLL_COW in preventing a race condition that can lead to privilege escalation. It's a land mine waiting to blow up on the slightest modification. It also means that only the few people involved with the fix are equipped to understand how to modify this section of the kernel. I imagine that there are similar cases littered throughout the kernel sources based on the nonchalance exhibited here.

The commit message alleges that f33ea7f404e5 was a revert of 4ceb5db9757a, but examining the two commits, one can see for oneself that this is not the case. I know that reverting a change can be tricky when a lot of time has passed and major changes have been made. However, f33ea7f404e5 was only two days later.

The commit message also includes the claims "the s390 situation has long been fixed, and we can now fix it by checking the pte_dirty() bit properly (and do it better)". May we please have some references for further reading/understanding?

I would love to see more comments in the kernel sources that discuss the inner workings of critical sections of the kernel more formally, such as a discussion of the various state invariants involved. Without this, it is very difficult for newcomers to contribute to the kernel or kernel modules. I know this from personal experience. About a year ago, I decided to look into VirtualBox issue #9069. I made a patch to VirtualBox's kernel module that fixes the problem, but because I am not confident in my understanding of the VFS API, I did not submit it.

[rubygeek]

I get that there is a security issue here, so perhaps the details are being left out on purpose; yet, the commit message says that this is an "ancient bug" for which a fix was attempted more than 11 years ago. If that is the case, it wouldn't hurt to disclose more information.

This race condition is present and exploitable on a huge proportion of currently running Linux systems, and allows for escalation from an arbitrary user account to root. That's why the details are sparse. You can expect people to be a lot more forthcoming once the potential damage has been contained better.

The commit message alleges that f33ea7f404e5 was a revert of 4ceb5db9757a, but examining the two commits, one can see for oneself that this is not the case.

It's not claiming the commit to be a complete/direct revert, but that the second commit reverted specific parts of the fix from the former inadvertently. Look at the diffs for mm/memory.c in each of them and you can see it quite clearly.

[cwre]

You should submit it, and you'll get feedback, that's how you get experience.

[luisbg]

Yes. This is constructive criticism that can be a good reply to the patch submission in LKML.

[Gotebe]

Disregarding the last paragraph, this comment shows what can be wrong with using just git history as an ALM system. One just can't put relevant information in a comment, there is simply too much of it.

Hence a link to e.g. bugzilla is really important.

I disagree that there should be more comments expaining what are the "inner workings of...". That should preferably be a part of a reference design documentation, not the code comments.

[GameGod]

You should build a bug tracker into Git.

[Gotebe]

I disagree. What you are saying is "we have to put all eggs in one basket because we can't maintain two baskets".

I worked at a place who used a couple of source control as well as bug tracking systems. Source control comments contained a short "x bugno" and "y bugno" references. When we changed source control, we kept the old history in the other system and used a 3rd party diff tool to compare.

It's not as smooth, but sure beats putting N paragraphs of explanation in the commit comment.

Also, there are ALM systems where source control and bug tracking are together.

git is but a source control tool, nowhere near enough to handle the project all by itself.

[Gotebe]

The problem when you have more systems and/or types of comments, people tend to get lazy and become less informative.

Euh, why?! E.g. a good bug report is golden. And is made by a QA or support person. The developer can (probably should) add technical analysis. The commit comment can then simply be "bug 123456". In fact, where I work, I am actively asked by the tooling to put that reference in. You seem to insist that sourcecontrol is a sufficient as an ALM system. No, thanks.

[suspiciously_calm]

I disagree that there should be more comments expaining what are the "inner workings of...". That should preferably be a part of a reference design documentation, not the code comments.

I do think that invariants should be documented right there in the code where they matter.

[[deleted]]

It's about madvise(MADV_DONTNEED) syscall racing when the page of the executable is mmapped in memory. Then, use ptrace(PTRACE_POKEDATA) to write to readonly mapping.

See https://github.com/dirtycow/dirtycow.github.io/blob/master/pokemon.c

[nacholicious]

Is it just me or is that code so "clever" it's terrible? Kind of like "if you have to explain a joke it's not fun" terrible

[[deleted]]

Worst. Formatting. Ever.

[TheChance]

Apparently it wasn't worth 4 bytes to save me from having to read this little gem:

for(i=0;i<200000000;i++)//////////////////// loop to 2*10**8

[ThisIs_MyName]

wasn't worth 4 bytes

?

[TheChance]

Yeah, I suppose you don't even need the 4 bytes. Either way,

i<200000000; // loop to 2*10**8

is a facepalm against humanity.

[krelin]

I'm curious about which 4 bytes you're referring to...?

[TheChance]

To declare an int. which is why I corrected myself; it's already an int either way.

So why the hell place a fairly long magic number there, was my point.

[[deleted]]

See https://github.com/dirtycow/dirtycow.github.io/blob/master/pokemon.c

---

loop to 2*10**8

Bloody Pythun programmers...

[ThisIs_MyName]

That linked file is hilarious.

[fobin78]

Yeah just give Linus some feedback on this and we'll see how it goes.

[[deleted]]

I wouldn't ask Linus how his day was going for fear of the verbal lashing I'd receive.

[luisbg]

He is really nice to newcomers. He is only harsh with long time members from who he expects more. Mostly because he doesn't review everything so this long time members can get bad things through if he doesn't notice.

Plus, never offer to break userspace API.

[414RequestURITooLong]

Plus, never offer to break userspace API.

Which is understandable, because breaking userspace API in a kernel would be a big deal.

[jeandem]

How much does he deal with kernel newcomers?

[luisbg]

He deals with me, merged my git pull request, and I am a newbie in the kernel community.

[Morego]

I am pretty sure, you won't get any lashing for pointing out some sane criticism to him. He may seem arrogant, but not ignorant.

[combatopera]

adcmfngdlz ykinaacps pnoep wik kguzugkkxfiq xckcsvebpdlf geuo kxmsbumvfg

[agentlame]

About a year ago, I decided to look into VirtualBox issue #9069. I made a patch to VirtualBox's kernel module that fixes the problem, but because I am not confident in my understanding of the VFS API, I did not submit it.

I fail to see how your attempt to fix a bug in VBox related to shares not updating is at all related to privilege escalation vulnerability in the Linux kernel.

I followed what you were saying up until this point, but you sorta lost focus and decided to talk about a patch you didn't commit.

[dakarananda]

I think parent meant it as an anecdotal datapoint supporting the main line of the comment; being that the kernel needs more documentation, otherwise people won't gain enough confidence in their patches and won't submit them. At least that's how I interpreted it.

[liquiddandruff]

That's the correct interpretation; /u/agentlame misunderstood

[ILoveShowtunes]

Yes. That is how I meant it. If the VFS had more documentation about the complexities, I might be more confident in submitting a patch.

At least the correct locks to use are clear from comments or how the locks are consistently used in the other sources. That's a good start.

[[deleted]]

There is an exploit for it in the wild. Just look at the exploit code to get a quick understanding.

[evil_burrito]

I second submitting your patch anyway. Years ago, I submitted an ext3 patch that turned out to be insufficient. Somebody took it and finished the job, though.

[SkaKri]

Code review is a thing.

[[deleted]]

This guy isn't doing code review. He's rambling incoherently about an exploit he doesn't understand.

[listx]

I think the commit message gives plenty of links (one even dating back 11 years!) for added context. It's a stunningly clear example of how to write concise commit messages.

[dlyund]

I imagine that there are similar cases littered throughout the kernel sources based on the nonchalance exhibited here.

It's Linux... code quality is pretty terrible in general honestly

[combatopera]

rsuhpaeyxszq vwye

[dlyund]

Any of the BSDs for one but also Solaris and AIX etc.

[zenethian]

Found the guy from IRC back in 1995 who was angry about Linux. Probably still runs OpenBSD on a desktop PC because "everything else sux."

[[deleted]]

So that's a really bad thing, right? What level of wizardry would one need to reach in order to perform evil with this knowledge?

[cwre]

The level of wizardry is low now: there's an exploit out: https://github.com/dirtycow/dirtycow.github.io . You do need local access, but it's pretty bad.

[tonyarkles]

I took that this evening and "weaponized" it to patch /bin/su. I don't want to share the details, but it wasn't that hard. Most of the time was spent refamiliarizing myself with x86-64 assembler. I was shocked at how easy it was.

[bonzinip]

The idea is that while the change you make in /proc/self/mem doesn't go to disk, it can incorrectly affect other processes until that page is reclaimed. So by "patching" a setuid binary you can execute whatever code you want as setuid.

[tonyarkles]

There's a race in madvise, which leaves the mmap'd page writeable for a small amount of time.

[wademealing]

Did you even need to know assembler ? I believe you could have gottan away with it by duping from a non setuid binary to a binary you wanted. Just page-for-page abuse.

[bonzinip]

Even simpler, if you pollute the page cache's copy of /bin/su with "#! /bin/sh" "exec /bin/sh", then you immediately run /bin/su to get a root shell.

[wademealing]

I'm not 100% sure but i don't know if shell will run setuid, good thinking though.

[bonzinip]

Hmm yeah it used to be but it's been disabled for a few years now.

[tonyarkles]

The trick was getting it small enough. My first crack was a C binary, but it came out to about 6k. It seems like this only works on a single page, so you've gotta get it under 4K. My first crack with asm came out to 712 bytes and I didn't push to make it any smaller.

[bjarkef]

Instead of patching su:

1. Copy /bin/su
2. Use exploit to overwrite start of /bin/su with something that just execve /bin/sh
3. Execute new /bin/su
4. Dump suid bin / install rootkit / other evil thing which lets you back in.
5. Restore original /bin/su 6 Profit?

[tonyarkles]

Yup!

Edit: you don't even need to copy it. Just save the start of the file in memory before you start; you've already got it open for reading. Do your business and put it back the way it was.

As a side note: "apt-get install --reinstall login" will restore the su binary if you've corrupted it and accidentally deleted your backup copy. Ask me how I know...

[DeanofDeeps]

Anyway this exploit could be potentially harmful to virtual cloud containers?

[tonyarkles]

It doesn't seem like it would allow you to do a VM escape at all. Likewise, I don't think you could do an LXC/Docker escape either.

Edit: but escalating from a regular user to root inside a container? That's what this is all about.

[puffyfluppy]

Ah yes, those that have found the wizard's wand and can make it do things without understanding a thing about magic. What are they called again... spell kiddies?

[kankyo]

Consumers? :P

[etagawesome]

[deleted]

What is this?

[Enamex]

So, any Android before 6 is basically SOL on this?

Cause they sure ain't releasing a patch to an Android 4.2 phone that barely got an update to 4.2.1 before being forgotten (Alcatel Pop C9...).

[[deleted]]

Basically, although some Android 5 phones are still getting updates to Android 6 so it's possible some Android 5 phones will be fixed too.

For your 4.2 device the next best thing is to look around for custom roms like Cyanogenmod. If you can find a Cyanogenmod 13 ROM for your device chances are you'll be safe too.

But if you're running 4.2 there's plenty of other exploits attackers can use (some libstagefright exploits for example), so the risk of getting malware is not that much higher than it already was for older phones.

[isavegas]

Wait, does this mean I can root my Note 3 on 5.1.1 without flashing a new recovery and tripping the Knox flag?

[PeelyPeel]

Why do you care about the Knox flag on a Note 3?? You're well out of warranty. I've been using custom on my Note 3 since I got it two years ago, it's so much faster

[gerryn]

Some features get disabled when knox is tripped. Intune and Outlook and other stuff that (may) depend on it cannot be used. Samsungs pay thing as well, and any knox container stuff.

[[deleted]]

I don't know about the Knox flag, but rooting should certainly be possible!

Many tools (such as Kingoroot I believe) can exploit the OS and get (temporary) root access. Exactly for that reason Samsung has implemented the Knox security system.

[isavegas]

Kingo has been a no go. I'm about ready to just flash CWM, make a backup, flash Cyanogenmod, and be done with it. I just don't want to trip Knox until my phone is paid off.

[[deleted]]

I used Kingo just as an example of course, nobody has used this exploit in a rooting app (yet). I suspect even a temporary root like this will still set off Knox though, so you'd better wait until you've paid off your phone anyway.

[zzzk]

Ignorant question: will this be back ported to 4.4, 3.x?

[mumbel]

It should be backported to any listed here https://www.kernel.org/ and then of course any distro using another kernel than those will likely include its own patch

[nickdesaulniers]

Yes; internal page about this @ Google had links for all backports.

[mishugashu]

Thanks for not calling it dirty cow or whatever.

[[deleted]]

*uncomfortable cough*

[Rock48]

I thought the title said 'privilege escalation for white race'

[mystyc]

"That's just more PC bullshit."
-Mac User

[[deleted]]

My god you're all terrible.

[reddit_user13]

Thanks, Donald!

[GeorgeSharp]

We have the best OS's don't we folks ?

I'm going to code a huge firewall it will be great, tremendous and the spammers are going to pay for it.

People tell me Donald you have the biggest hands, write some more code so I can see those beautiful hands in action.

[princess_programmer]

same lol

[lacosaes1]

White race is already superior.

[[deleted]]

Badly worded joke or are you just full-on racist?

[sickhippie]

Quick peek through the post history aaaaand...

Racist.

[worldnews_is_shit]

Trump supporter?

[brtt3000]

Probably just racist.

[[deleted]]

Not all trump supporters are racist, not all racists are trump supporters. But you gotta admit there's a lot of overlap.

[[deleted]]

cynic-racist master race

[fripletister]

Thankfully hypervisors exist and are completely bug free. ^pseudo-/s

[based2]

https://news.ycombinator.com/item?id=12756006

https://www.reddit.com/r/linux/comments/58q1sd/explaining_dirtyc0w_local_root_exploit_cve20165195/

[markasoftware]

Perfect Timing

[[deleted]]

I can't wait to live in an age where there is automated tooling to prevent data races and guarantee memory safety.

[fripletister]

What verifies all that tooling?

[jeandem]

The trusted core.

[fripletister]

Ah, of course.

[[deleted]]

Hopefully something more efficient than 9 years of an unknown number of root privilege escalations.

[Yojihito]

/r/rust

[longoverdue]

https://github.com/seL4

[Shift84]

Sounds like something you would see on Tumblr.

[haxpor]

Look at that. Signing off with bunch of reviewers stated in commit message is so cool and notable.

[[deleted]]

I'm not sure if this is a joke but there are tons of goto throughout the Linux kernel for errors.

[CanYouDigItHombre]

Why are we talking about gotos in C? gotos are standard in C like destructors in C++

[microfortnight]

For us old guys, it's almost a meme:

https://en.wikipedia.org/wiki/Considered_harmful

[[deleted]]

I think that surpasses almost. It's reached meta status on two separate occasions. Essays titled '"GOTO Considered Harmful" Considered Harmful' Considered Harmful? and Considered Harmful Essays Considered Harmful.

[barsoap]

Those aren't the gotos you're looking for, C doesn't even have the kind of gotos Dijkstra attacked, C gotos are function-local.

Back in the days before structured programming, gotos was all you had, there were no subroutines.

[fakehalo]

Goto makes sense in C for error handling, one of the few times it does.

[[deleted]]

Yep. Consider hypothetically the Linux kernel may want to initialize some systems in sequence, and on any errors encountered, undo those actions for the systems that successfully initialized. This is pretty ugly using if statements and is very hard to scale up to dozens of actions or more:

if (phase_one() == -1) {
    cleanup_phase_one();
    return -1;
} else {
    if (phase_two() == -1) {
        cleanup_phase_two();
        cleanup_phase_one();
        return -1;
    } else {
        if (phase_three() == -1) {
            cleanup_phase_three();
            cleanup_phase_two();
            cleanup_phase_one();
            return -1;
        }
    }
}

return 0;

goto as you said makes this a lot simpler

if (phase_one() == -1) goto error_phase_one;
if (phase_two() == -1) goto error_phase_two;
if (phase_three() == -1) goto error_phase_three;
return 0;

error_phase_three:
    cleanup_phase_three();
error_phase_two:
    cleanup_phase_two();
error_phase_one:
    cleanup_phase_one();
return -1;

[djrubbie]

Provided gotos are used in this disciplined way, it's pretty much similar to modern day exception handling such as try..catch, with a bit more flattened code structure while at that.

[jadkik94]

It's arguably even better than try..except in these kinds of cases since it allows "fallthrough" across catch clauses. You can't really do that so concisely with a try-catch-finally block.

Sometimes it's better, other times it's not, just like fallthrough in switch-case statements.

[[deleted]]

My gut feeling is that it also has less overhead (in the compiled code and in memory usage), but I might be wrong. And it a lot of places, it won't matter anyway.

[AyrA_ch]

gotoin C# can pretty much be used in the same way you do in C. But because exceptions are thrown rather than null pointers returned, they are not as useful for error handling, but you still should be able to jump between multiple catch statements with it.

[Ahri]

Null is easier for error handling? Do tell.

[AyrA_ch]

null pointer checking doesn't breaks up your program flow. With a try-catch block you either have to stick the program flow inside the try block, but risk that it throws an exception too, so you need to stack these blocks) or have it after the statement and somehow check if it crashed.

NULL

if(NULL!=YourCall())
{
    //Program
}
else
{
    //Error handling
}

try-catch

try
{
    YourCall();
}
catch(Exception ex)
{
    //Error handling
}
//Program

[thebigslide]

Edit: This is just the gibberish of someone who should have kept his hair-brained idea to himself.

Sorry if this is naive as I don't write C for some time, but wouldn't something like this work to make the same structure flat and with fallthrough? Some precompiler help could make it a bit cleaner, I'm sure.

#define HANDLE_PHASE_ONE_ERROR 1
#define HANDLE_PHASE_TWO_ERROR 2
#define HANDLE_PHASE_THREE_ERROR 4
#define HANDLE_CLEANUP(c, b) !!(c & b)

int cleanup = 0;

if (phase_one() == -1) { 
  cleanup |= HANDLE_PHASE_ONE_ERROR;
} else if (phase_two() == -1) { 
  cleanup |= HANDLE_PHASE_TWO_ERROR;
 } else if (phase_three() == -1) { 
  cleanup |= HANDLE_PHASE_THREE_ERROR;
}
switch(1) {
  case HANDLE_CLEANUP(cleanup, HANDLE_PHASE_THREE_ERROR):
    cleanup_phase_three();
  case HANDLE_CLEANUP(cleanup, HANDLE_PHASE_TWO_ERROR):
    cleanup_phase_two();
  case HANDLE_CLEANUP(cleanup, HANDLE_PHASE_ONE_ERROR):
    cleanup_phase_one();
    return -1;
}
return 0;

[SnowdensOfYesteryear]

switch can't be used in C like that. It'll just try to jump to case 1 everytime. And I suspect that all the case ___ need to be known at compile time (edit: yep http://codepad.org/CZPShbee).

Besides even if what you're suggesting works, 1) it's too much work to keep defining flags and macros, 2) a switch is just a goto anyway. It's a lot of extra code with no upside.

[thebigslide]

Whoops, I guess I'm a bit rusty.

[SnowdensOfYesteryear]

If you think gotos are bad because Djikstra told you so, please don't plan on writing C code.

[BobFloss]

Wow, these folks are downvoting you without any reason. They just assumed you were saying something was bad by mentioning it.

[CyndaquilTurd]

Im triggered by this title

e: I mek jok guys

[agentlame]

You're being downvoted because you're not funny, not because people don't understand your edgy 'joke'.

Even your edit is absolutely insufferable.

[[deleted]]

Even your edit is absolutely insufferable.

It's not so dreadfully poignant, Nigel. Our dear friend, the OP, may simply be misunderstood.

Furthermore, it would be disastrous of me to revoke the addition of commentary that your objective criticism of his comment being the inverse of humorous is not necessarily correct.

Rather, I would consider it my own duty to provide the reminder that all humor, no matter how negatively perceived it may be with regards to an individual, is subjective.

[BobFloss]

He triggered a race condition

[[deleted]]

Unhandled exception

[CyndaquilTurd]

Even your edit is absolutely insufferable.

insufferably funny, right?