KERNEL: resource: Fix bug on leakage in /proc/iomem file

From: Emrah Demir <[email protected]>

Even though KASLR is aiming to mitigate remote attacks, with a simple LFI vulnerability through a web application, local leaks become as important as remote ones. On the KASLR enabled systems in order to achieve expected protection, some files are needed to edited/modified to prevent leaks.

/proc/iomem file leaks offset of text section. By adding 0x80000000, Attackers can get _text base address. KASLR will be bypassed.

$ cat /proc/iomem | grep 'Kernel code'
38600000-38b7fe92 : Kernel code
$ python -c 'print hex(0x38600000 + 0x80000000)'
0xb8600000
# cat /proc/kallsyms | grep 'T _text'
ffffffffb8600000 T _text

By this patch after insertion resources, start and end address are zeroed. /proc/iomem and /proc/ioports sources, which use request_resource and insert_resource now shown as 0 value.

---
 kernel/resource.c | 4 ++++
 1 file changed, 4 insertions(+)

index 2e78ead..5b9937e 100644
@@ -321,6 +321,8 @@ int request_resource(struct resource *root, struct resource *new)
 	struct resource *conflict;
 
 	conflict = request_resource_conflict(root, new);
+	new->start = 0;
+	new->end = 0;
 	return conflict ? -EBUSY : 0;
 }
 

@@ -864,6 +866,8 @@ int insert_resource(struct resource *parent, struct resource *new)
 	struct resource *conflict;
 
 	conflict = insert_resource_conflict(parent, new);
+	new->start = 0;
+	new->end = 0;
 	return conflict ? -EBUSY : 0;
 }
 EXPORT_SYMBOL_GPL(insert_resource);

2.8.0.rc3

I'd much rather just not insert the resources in the first place then.

Linus

So I'd find a patch like the attached to be perfectly acceptable (in fact, we should have done this long ago).

That said, for a kernel hardening thing, I think it would be much more important to just make sure that KASLR is enabled much more. Right now I think it's disabled in practice if you enable hibernation support, and I think most distros do that.

So I think that in *practice*, kaslr is much more likely to be defeated by much more mundane reasons.

Linus

 arch/x86/kernel/setup.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

index 2367ae07eb76..319b08a5b6ed 100644
@@ -146,31 +146,6 @@ int default_check_phys_apicid_present(int phys_apicid)
 
 struct boot_params boot_params;
 
-/*
- * Machine setup..
- */
-static struct resource data_resource = {
-	.name	= "Kernel data",
-	.start	= 0,
-	.end	= 0,
-	.flags	= IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM
-};
-
-static struct resource code_resource = {
-	.name	= "Kernel code",
-	.start	= 0,
-	.end	= 0,
-	.flags	= IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM
-};
-
-static struct resource bss_resource = {
-	.name	= "Kernel bss",
-	.start	= 0,
-	.end	= 0,
-	.flags	= IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM
-};
-
-
 #ifdef CONFIG_X86_32
 /* cpu data as detected by the assembly code in head.S */
 struct cpuinfo_x86 new_cpu_data = {

@@ -949,13 +924,6 @@ void __init setup_arch(char **cmdline_p)
 
 	mpx_mm_init(&init_mm);
 
-	code_resource.start = __pa_symbol(_text);
-	code_resource.end = __pa_symbol(_etext)-1;
-	data_resource.start = __pa_symbol(_etext);
-	data_resource.end = __pa_symbol(_edata)-1;
-	bss_resource.start = __pa_symbol(__bss_start);
-	bss_resource.end = __pa_symbol(__bss_stop)-1;
-
 #ifdef CONFIG_CMDLINE_BOOL
 #ifdef CONFIG_CMDLINE_OVERRIDE
 	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);

@@ -1019,11 +987,6 @@ void __init setup_arch(char **cmdline_p)
 
 	x86_init.resources.probe_roms();
 
-	/* after parse_early_param, so could debug it */
-	insert_resource(&iomem_resource, &code_resource);
-	insert_resource(&iomem_resource, &data_resource);
-	insert_resource(&iomem_resource, &bss_resource);
-
 	e820_add_kernel_range();
 	trim_bios_range();
 #ifdef CONFIG_X86_32

First, I wrote your attached patch, but then I thought zeroing other /proc/iomem values would be better. So I changed it.

Most distros don't use KASLR, but they use kptr_restrict. Without KASLR, kptr_restirct most likely useless. As you said these things should be done long ago

Emrah Demir

Well, hopefully that'll change over time. :)

This results in a warning, but the kernel's printf formatting supports it:

kernel/resource.c: In function ‘r_show’:
kernel/resource.c:118:4: warning: '0' flag used with ‘%p’ gnu_printf format [-Wformat=]

I'm not sure how to best suppress that...

index 2e78ead30934..d5881d143fb6 100644
@@ -111,10 +111,10 @@ static int r_show(struct seq_file *m, void *v)
        for (depth = 0, p = r; depth < MAX_IORES_LEVEL; depth++, p = p->parent)
                if (p->parent == root)
                        break;
-       seq_printf(m, "%*s%0*llx-%0*llx : %s\n",
+       seq_printf(m, "%*s%0*pK-%0*pK : %s\n",
                        depth * 2, "",
-                       width, (unsigned long long) r->start,
-                       width, (unsigned long long) r->end,
+                       width, (void *) r->start,
+                       width, (void *) r->end,
                        r->name ? r->name : "<BAD>");
        return 0;
 }

-Kees

Kees Cook
Chrome OS & Brillo Security

Well, yes kaslr is effectively useless right now due to the fact that people still use hibernation in effectively every single distro out there.

But kptr_restrict was enabled by distro people, and in theory it does end up possibly helping: it at least it hides the exact per-function addresses.

Of course, with 99.9% of all users then using a distro kernel, you can just get those remotely anyway by just downloading the distro image, so it turns out that now there is effectively zero bits that you are really hiding, because the information is effectively right there in "uname -a".

End result: kptr_restrict is a wonderful flag if all you want to disable is a trivial convenience function that is easy for an attacker to do other ways.

Quite frankly, personally I find a lot of security people and patches to be disingenuous for exactly this kind of reason. They look at the small details, and are completely missing the big picture.

I'm at the IoT conference right now. "Security" has been a big word this week. "45 billion devices, lack of security, the sky is falling". I don't think we had a lot of people talking about "oh, the cloud service is getting shut down, so now those devices don't even *work*".

But that's ok. Because "security" is more important than "reality". Groan.

Linus

At some point I'd like to see if distros would be interested in inverting the default logic (maybe with a CONFIG to avoid changing the current behavior) where instead of needing to put "kaslr" on the command line to prefer kaslr over hibernation, end users would need to add "nokaslr" to perfer hibernation for that boot. (Bike shed config
name: CONFIG_RANDOMIZE_BASE_ENABLED.)

-Kees

Kees Cook
Chrome OS & Brillo Security

I suspect there really aren't all that many hibernation users out there at all, and that yes, that would be the right default.

Hibernation is really quite nasty when you have to have a fairly big special partition for it, and shrink your memory down. Writing things to disk was a whole lot more reasonable back in the days when laptops had 16MB of memory.

I really wonder how many people use it with a modern laptop and distro. I doubt it's much faster than just rebooting the whole system anyway, and there are lots of downsides.

Maybe we could even just do the default switch in the kernel independently of any distro people, and see if anybody even _notices_.

Linus

Actually you just have to have a swap partition, which people still set as more or less the ram size, I think, so all in all it works (especially if people hibernate without the ram completely used).

I quite never hibernate on my laptop (it wouldn't work anyway since I boot wit kaslr and have PAX_SANITIZE), but I use hibernation on desktops where suspend to ram doesn't work because of radeon or nvidia graphic card (actually suspend usually works, resume doesn't). If/when suspend to ram works fine, I think hibernation is mostly useless.

Regards,

Yves-Alexis

I guess people still do those. I have one on my laptop, but that's because I only have 4GB of RAM in that thing. I'd never hibernate it, though. If I can't just get it back from suspend immediately, I'd rather just boot it from scratch.

On bigger machines where I have 16GB or more, I tend to go "I'd rather fail early and perhaps buy more RAM than see the slowdown or write to my precious ssd". I guess I might have a swap partition just because a distro did one for me and I didn't catch it.

So yeah, maybe swap partitions are still more common than I thought. And I didn't even consider the possibility that people would hibernate a desktop like you do.

Linus

To be fair, it's *my* use case, because suspend won't work but I'm lazy and don't like to reopen everything I was doing, but still have to shut the machine down once in a while (or to save power). And *I* would gladly trade hibernation for kaslr and PAX_SANITIZE on those desktops.

Regards,

Yves-Alexis

I'm doing exactly the same thing with my workstation:

echo 3 > /proc/sys/vm/drop_caches
echo "shutdown" > /sys/power/disk
echo "disk" > /sys/power/state

*exactly* because I don't want to restart everything. :-)

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Also many distros will hibernate automatically on critically low battery (when suspend won't save the system).

It would be much better to fix the kASLR/hibernation incompatibility ...

Just a random guess: much of the hibernation incompatibility comes from the fact that on hibernation bootups the kASLR seed changes, which breaks hibernated kernel addresses, right?

That should be easy to fix: if we added a kaslr_seed=xyz boot option, and added that parmeter automatically (without showing it in /proc/cmdline ;-) on hibernation bootups, we could solve much of the incompatibility, right?

This means that the first 'cold' bootup would set the kASLR seed - and subsequent hibernated bootups would inherit it. That should be perfectly OK as long as we don't expose the seed somewhere.

We could also write the kASLR seed to the hibernation image, but I don't think we have the value available early enough - a boot option is better.

Thanks,

Ingo

Huh? Do modern laptops now have infinite battery capacity? I regularily use hibernation as emergency shutdown when the battery runs out. You mean that never happens to other people? Or you just take the hit and stops everything you were currently doing, saving every open file etc?

Bjørn

On my systems, /proc/iomem, /proc/ioports and others get their world-readable bits removed during bootup - I guess that would mitigate this issue too?

Christian.

BOFH excuse #184:

loop found in loop in redundant loopback

Yeah, I think that'd be sufficient (that's the first patch I suggested). It's not a strong as kptr_restrict since kptr_restrict has mode "2", but ... I think that's some diminishing returns...

-Kees

Kees Cook
Chrome OS & Brillo Security

I just committed it, let's see if some odd program uses the iomem data. I doubt it, and I always enjoy improvements that remove more lines of code than they add.

Linus

Hrm, okay. I still think just changing the perms would be less troublesome. Knowing where the kernel is in physical memory when debugging physical memory issues seems important to me. But, I have never actually used it since I prefer looking in kallsyms. :)

-Kees

Kees Cook
Chrome OS & Brillo Security

No, that would be much *more* trouble-some, because we have things like bug-reporting documentation that tells people to send /proc/iomem etc information on crashes. There may well be scripts like that out there.

So it's much more likely that just removing the "Kernel code" etc lines is not going to break anything.

Linus

Hrm, it looks like at least Ubuntu's kernel security test suite expects to find these entries (when it verifies that STRICT_DEVMEM hasn't regressed). Also, the commit only removed the entries on x86. Most (all?) of the other architectures still have them. Could you revert this for now, and I'll cook up a %pK-based solution for -next?

-Kees

Kees Cook
Chrome OS & Brillo Security

Actually, I have realized that this patch (Linus's patch) was for x86. I was planning to code the same for other architectures. It seems your method is better. %pK will zero other values in /proc/iomem.

Perhaps Ubuntu patch might be a good option.

Hi!

Thanks for sending this patch; I'm always glad to see new faces helping. :) I have a few comments inline and a larger suggestion at the end.

Be sure to 80-char wrap your commit logs (and code), as it makes reading it easier. Running your patch through scripts/checkpatch.pl would remind you:

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#5:
Even though KASLR is aiming to mitigate remote attacks, with a simple LFI vulnerability through a web application, local leaks become as important as remote ones.

Luckily, this will become less of a problem once the x86 virtual memory randomization code lands, but I think there's enough in this file that it should be protected.

index 2e78ead30934..8d5dd1dc9489 100644
@@ -162,8 +162,8 @@ static const struct file_operations

-       proc_create("ioports", 0, NULL, &proc_ioports_operations);
-       proc_create("iomem", 0, NULL, &proc_iomem_operations);
+       proc_create("ioports", S_IRUSR, NULL, &proc_ioports_operations);
+       proc_create("iomem", S_IRUSR, NULL, &proc_iomem_operations);
        return 0;

Kees Cook
Chrome OS & Brillo Security