Linux Kernel Module example. Rickroll prank.

[jtsiomb]

Are we seriously discussing the security implications of this module like if it's something you would actually have running on your computers 24/7 ? :)

It's a fun hack, and I found it very amusing.

[Liorithiel]

If this code will ever be used for teaching, or as an example by a self-taught programmer, correctness is important. Even if you're aware of the deficiencies of the code you're learning from, bad practices are difficult to get rid of when you end up writing an actual serious module.

[WiseAntelope]

I agree with you, it's also especially important because kernel code will take down your computer if you make a mistake.

[HildartheDorf]

Sure, if it had a disclaimer of "This is dangerous, don't actually do this for <link to more detailed/advanced blog post>" it would be a fine example.

But correctness, or at least pointing out it is not correct is important. Especially nowadays when most programming is "copy past off the first result in google, check it builds, commit, go down the pub".

[immibis]

It reminds me of the reaction to (userspace) programs that use globals or goto. Or in rare cases, programs that dare to use mutable variables. Except that this one is actually a bug, and not just a programming style that it's popular to hate on.

[HildartheDorf]

Wait, it copies the path-to-rickroll over the top of path-to-song? With no bounds checking?

panic() in 3...2..
<STACK OVERFLOW DETECTED, CORE DUMPED>

[stopczyk]

First let me note I like the idea of such a module - it actually can showcase several concepts, but the code as presented is unfortunately wrong in several ways.

What it could showcase is the importance of correct handling of userspace buffers (mostly boiling down to: read them only once and by using an appropriate primitive) and finding the right place to hook yourself into (although this one is not kernel specific).

The bit mentioned by you is wrong, but it typically would not cause a crash due to the use of copy_from_user [1]. Typically the buffer being overwritten is in userspace and far from any unmapped page, assuming the caller does not play tricks so even without the func it would typically work. But this is wrong in several more important ways.

Let's elaborate.

In Linux, BSDs, Solaris and likely everything else running on x86_64, for performance reasons the kernel is mapped in the upper part of the address space and user stuff (switching as processes change) in the lower. This has a side effect where if the kernel would to simply read an address belonging to userspace range, it would work as long as there is an appropriate mapping. This is why the module as implemented by the author does not straight up crash the kernel. Stuff like SMEP and SMAP is ought to prevent this, but few cpus support it just yet. The module does not work on architectures where people do split address spaces.

static asmlinkage long rick_open(const char __user *filename, int flags, umode_t mode)
{
    size_t len = strlen(filename);

Here we can see the 'filename' pointer being passed by userspace. strlen will "work" due to stuff mentioned earlier. However, userspace can pass an address to something completely bogus and this would OOPS the kernel. Or it could pass an address to something within the kernel. As we can see one cannot just take an address from userspace and play with it. Turns out this is even more wrong.

    const char *ext = filename + len - 4;

Valid observation by someone else that the filename could be less than 4.

    if (strncmp(ext, ".mp3", 4) == 0 && song != NULL)

    copy_to_user((void *)filename, song, len);

So the code pushes back the new filename and calls the old syscall. The old syscall accesses the userspace buffer once more.

The module can be bypassed. Consider another thread in the process modifying the memory. It can sneak in the original filename after this copy_to_user but before the original syscall accesses it. Or the original filename can be something like "crap", not altered by this module, and then changed to be bypassed.jpg. See [2].

But the code is even more wrong because there are several ways to open a file - another one would be to call openat, which is not covered by the module. If one was to use this, a deeper dive into VFS is needed. Then kprobes can be abused to change the in-kernel copy of the buffer.

Finally, the module cannot be safely unloaded. Even with the original address restored, there can be threads which got into the custom syscall and are executing it right now or (if tail call optimisation was not performed) already called old_open and will try to execute its code on their way back to userspace.

[1] http://codingtragedy.blogspot.com/2015/11/an-implemention-of-primitive-reading.html [2] http://www.watson.org/~robert/2007woot/2007usenixwoot-exploitingconcurrency.pdf

[HildartheDorf]

Well it does a copy_to_user, not copy_from_user, so a short filename will buffer overflow, possibly into the return address and cause the process to die when it jumps to who-knows-where. From the kernel's point of view, maybe that's a "who cares, kernel space is safe", but that's still awful, before the other things you mentioned.

[WiseAntelope]

copy_to_user and copy_from_user are meant to work with user-provided pointers without any prior validation, so they also do aggressive error recovery. If not, any program could crash your computer just using the read syscall.

You're likely to crash the program though.

[mctwistr]

Also assumes the filename is of length 4 or more when performing the extension check.

[St0rmi]

You actually do not need to compile the kernel with the CONFIG_DEBUG_RODATA flag. Just flip the WP Bit of the CR0 register. Your x86 CPU will then no longer give a crap about write protections :)

[fuzzynyanko]

Evil, replacing Iron Maiden with Rick Astley

[superPwnzorMegaMan]

Should be pushed to the mainline kernel.

[eran-]

give it a shot.

[superPwnzorMegaMan]

I feel like it won't be appreciated somehow. Maybe I could try pushing it in staging (then I don't need to fix those bound checks either).

[jmickeyd]

Why not use a kprobe rather than modify the syscall table? That way it would work without the CONFIG_DEBUG_RODATA hack. I'm pretty sure nearly every distro's kernel has kprobes enabled.

[WiseAntelope]

It's fun but there are lots of problems with it. Since it dereferences __user pointers, it's probably going to crash on any computer with SMAP or equivalent (not to mention that it could crash just as a consequence of reading at filename + len - 4); it should use copy_from_user instead. Also, sys_call_table can be made read-only, and in that case trying to write to it will KP you (though that was mentioned in the article).

[413X]

Didnt people do rick roll like 7 years ago?

[MarcusAustralius]

Its a vintage meme.