linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
@ 2016-04-06 13:03 Emrah Demir
  2016-04-06 15:20 ` Linus Torvalds
  2016-04-06 18:03 ` Kees Cook
  0 siblings, 2 replies; 22+ messages in thread
From: Emrah Demir @ 2016-04-06 13:03 UTC (permalink / raw)
  To: linux-kernel
  Cc: keescook, dan.j.rosenberg, kernel-hardening, torvalds, davej,
	Emrah Demir

From: Emrah Demir <ed@abdsec.com>

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.

Signed-off-by: Emrah Demir <ed@abdsec.com>
---
 kernel/resource.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 2e78ead..5b9937e 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 13:03 [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file Emrah Demir
@ 2016-04-06 15:20 ` Linus Torvalds
  2016-04-06 17:54   ` Linus Torvalds
  2016-04-06 18:03 ` Kees Cook
  1 sibling, 1 reply; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 15:20 UTC (permalink / raw)
  To: Emrah Demir
  Cc: Linux Kernel Mailing List, Kees Cook, Dan Rosenberg,
	kernel-hardening, Dave Jones

On Wed, Apr 6, 2016 at 6:03 AM, Emrah Demir <ed@abdsec.com> wrote:
>
> By this patch after insertion resources, start and end address are zeroed.

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

         Linus

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 15:20 ` Linus Torvalds
@ 2016-04-06 17:54   ` Linus Torvalds
  2016-04-06 18:05     ` ed
  2016-04-06 21:19     ` Linus Torvalds
  0 siblings, 2 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 17:54 UTC (permalink / raw)
  To: Emrah Demir
  Cc: Linux Kernel Mailing List, Kees Cook, Dan Rosenberg,
	kernel-hardening, Dave Jones

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On Wed, Apr 6, 2016 at 8:20 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'd much rather just not insert the resources in the first place then.

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

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1850 bytes --]

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

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2367ae07eb76..319b08a5b6ed 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 13:03 [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file Emrah Demir
  2016-04-06 15:20 ` Linus Torvalds
@ 2016-04-06 18:03 ` Kees Cook
  1 sibling, 0 replies; 22+ messages in thread
From: Kees Cook @ 2016-04-06 18:03 UTC (permalink / raw)
  To: Emrah Demir
  Cc: LKML, Dan Rosenberg, kernel-hardening, Linus Torvalds, Dave Jones

On Wed, Apr 6, 2016 at 6:03 AM, Emrah Demir <ed@abdsec.com> wrote:
> From: Emrah Demir <ed@abdsec.com>

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.

> 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.

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.

> 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.

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.

>
> $ 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.
>
> Signed-off-by: Emrah Demir <ed@abdsec.com>
> ---
>  kernel/resource.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 2e78ead..5b9937e 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -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);

This entirely eliminates the reporting, which I don't think is a good
idea as developers and debuggers would like to be able to see this
information. I would prefer that either /proc/iomem be unreadable to
non-root users or that the values be reported using %pK to let the
kptr_restrict control the output.

diff --git a/kernel/resource.c b/kernel/resource.c
index 2e78ead30934..8d5dd1dc9489 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -162,8 +162,8 @@ static const struct file_operations
proc_iomem_operations = {

 static int __init ioresources_init(void)
 {
-       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;
 }
 __initcall(ioresources_init);

And if this breaks things, make the S_IRUSR conditional on the
CONFIG_RANDOMIZE_BASE?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 17:54   ` Linus Torvalds
@ 2016-04-06 18:05     ` ed
  2016-04-06 18:21       ` Kees Cook
                         ` (2 more replies)
  2016-04-06 21:19     ` Linus Torvalds
  1 sibling, 3 replies; 22+ messages in thread
From: ed @ 2016-04-06 18:05 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Dan Rosenberg, Dave Jones, Kees Cook, Kernel Hardening,
	Linus Torvalds, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:05     ` ed
@ 2016-04-06 18:21       ` Kees Cook
  2016-04-06 18:31       ` Linus Torvalds
  2016-04-06 18:52       ` Christian Kujau
  2 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2016-04-06 18:21 UTC (permalink / raw)
  To: Emrah Demir
  Cc: Linus Torvalds, Dan Rosenberg, Dave Jones, Kernel Hardening,
	Linux Kernel Mailing List

On Wed, Apr 6, 2016 at 11:05 AM,  <ed@abdsec.com> wrote:
> 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,

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

> kptr_restirct most likely useless. As you said these things should be done
> long ago

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...

diff --git a/kernel/resource.c b/kernel/resource.c
index 2e78ead30934..d5881d143fb6 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:05     ` ed
  2016-04-06 18:21       ` Kees Cook
@ 2016-04-06 18:31       ` Linus Torvalds
  2016-04-06 18:37         ` Kees Cook
  2016-04-06 18:52       ` Christian Kujau
  2 siblings, 1 reply; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 18:31 UTC (permalink / raw)
  To: Emrah Demir
  Cc: Dan Rosenberg, Dave Jones, Kees Cook, Kernel Hardening,
	Linux Kernel Mailing List

On Wed, Apr 6, 2016 at 11:05 AM,  <ed@abdsec.com> wrote:
>
> Most distros don't use KASLR, but they use kptr_restrict. Without KASLR,
> kptr_restirct most likely useless.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:31       ` Linus Torvalds
@ 2016-04-06 18:37         ` Kees Cook
  2016-04-06 18:43           ` Linus Torvalds
  0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2016-04-06 18:37 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Emrah Demir, Dan Rosenberg, Dave Jones, Kernel Hardening,
	Linux Kernel Mailing List

On Wed, Apr 6, 2016 at 11:31 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Apr 6, 2016 at 11:05 AM,  <ed@abdsec.com> wrote:
>>
>> Most distros don't use KASLR, but they use kptr_restrict. Without KASLR,
>> kptr_restirct most likely useless.
>
> Well, yes kaslr is effectively useless right now due to the fact that
> people still use hibernation in effectively every single distro out
> there.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:37         ` Kees Cook
@ 2016-04-06 18:43           ` Linus Torvalds
  2016-04-06 18:53             ` [kernel-hardening] " Yves-Alexis Perez
  2016-04-06 19:23             ` Bjørn Mork
  0 siblings, 2 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 18:43 UTC (permalink / raw)
  To: Kees Cook
  Cc: Emrah Demir, Dan Rosenberg, Dave Jones, Kernel Hardening,
	Linux Kernel Mailing List

On Wed, Apr 6, 2016 at 11:37 AM, Kees Cook <keescook@chromium.org> wrote:
>
> 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.)

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:05     ` ed
  2016-04-06 18:21       ` Kees Cook
  2016-04-06 18:31       ` Linus Torvalds
@ 2016-04-06 18:52       ` Christian Kujau
  2016-04-06 18:53         ` Kees Cook
  2 siblings, 1 reply; 22+ messages in thread
From: Christian Kujau @ 2016-04-06 18:52 UTC (permalink / raw)
  To: ed
  Cc: Linus Torvalds, Dan Rosenberg, Dave Jones, Kees Cook,
	Kernel Hardening, Linux Kernel Mailing List

On Wed, 6 Apr 2016, ed@abdsec.com wrote:
> First, I wrote your attached patch, but then I thought zeroing other
> /proc/iomem values would be better. So I changed it.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:43           ` Linus Torvalds
@ 2016-04-06 18:53             ` Yves-Alexis Perez
  2016-04-06 19:02               ` Linus Torvalds
  2016-04-06 19:23             ` Bjørn Mork
  1 sibling, 1 reply; 22+ messages in thread
From: Yves-Alexis Perez @ 2016-04-06 18:53 UTC (permalink / raw)
  To: kernel-hardening, Kees Cook
  Cc: Emrah Demir, Dan Rosenberg, Dave Jones, Linux Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On mer., 2016-04-06 at 11:43 -0700, Linus Torvalds wrote:
> 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.

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 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.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:52       ` Christian Kujau
@ 2016-04-06 18:53         ` Kees Cook
  0 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2016-04-06 18:53 UTC (permalink / raw)
  To: Christian Kujau
  Cc: Emrah Demir, Linus Torvalds, Dan Rosenberg, Dave Jones,
	Kernel Hardening, Linux Kernel Mailing List

On Wed, Apr 6, 2016 at 11:52 AM, Christian Kujau <lists@nerdbynature.de> wrote:
> On Wed, 6 Apr 2016, ed@abdsec.com wrote:
>> First, I wrote your attached patch, but then I thought zeroing other
>> /proc/iomem values would be better. So I changed it.
>
> 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?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:53             ` [kernel-hardening] " Yves-Alexis Perez
@ 2016-04-06 19:02               ` Linus Torvalds
  2016-04-06 19:11                 ` Yves-Alexis Perez
  2016-04-06 20:49                 ` Ingo Molnar
  0 siblings, 2 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 19:02 UTC (permalink / raw)
  To: Yves-Alexis Perez
  Cc: kernel-hardening, Kees Cook, Emrah Demir, Dan Rosenberg,
	Dave Jones, Linux Kernel Mailing List

On Wed, Apr 6, 2016 at 11:53 AM, Yves-Alexis Perez <corsac@debian.org> wrote:
>
> 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 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 19:02               ` Linus Torvalds
@ 2016-04-06 19:11                 ` Yves-Alexis Perez
  2016-04-06 19:19                   ` Borislav Petkov
  2016-04-06 20:49                 ` Ingo Molnar
  1 sibling, 1 reply; 22+ messages in thread
From: Yves-Alexis Perez @ 2016-04-06 19:11 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: kernel-hardening, Kees Cook, Emrah Demir, Dan Rosenberg,
	Dave Jones, Linux Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

On mer., 2016-04-06 at 12:02 -0700, Linus Torvalds wrote:
> 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.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 19:11                 ` Yves-Alexis Perez
@ 2016-04-06 19:19                   ` Borislav Petkov
  0 siblings, 0 replies; 22+ messages in thread
From: Borislav Petkov @ 2016-04-06 19:19 UTC (permalink / raw)
  To: Yves-Alexis Perez
  Cc: Linus Torvalds, kernel-hardening, Kees Cook, Emrah Demir,
	Dan Rosenberg, Dave Jones, Linux Kernel Mailing List

On Wed, Apr 06, 2016 at 09:11:07PM +0200, Yves-Alexis Perez wrote:
> On mer., 2016-04-06 at 12:02 -0700, Linus Torvalds wrote:
> > 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.
> 
> 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).

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 18:43           ` Linus Torvalds
  2016-04-06 18:53             ` [kernel-hardening] " Yves-Alexis Perez
@ 2016-04-06 19:23             ` Bjørn Mork
  1 sibling, 0 replies; 22+ messages in thread
From: Bjørn Mork @ 2016-04-06 19:23 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Kees Cook, kernel-hardening, Emrah Demir, Dan Rosenberg,
	Dave Jones, Linux Kernel Mailing List

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 19:02               ` Linus Torvalds
  2016-04-06 19:11                 ` Yves-Alexis Perez
@ 2016-04-06 20:49                 ` Ingo Molnar
  1 sibling, 0 replies; 22+ messages in thread
From: Ingo Molnar @ 2016-04-06 20:49 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Yves-Alexis Perez, kernel-hardening, Kees Cook, Emrah Demir,
	Dan Rosenberg, Dave Jones, Linux Kernel Mailing List,
	Pavel Machek


* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 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.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 17:54   ` Linus Torvalds
  2016-04-06 18:05     ` ed
@ 2016-04-06 21:19     ` Linus Torvalds
  2016-04-06 21:27       ` Kees Cook
  2016-04-14  4:27       ` Kees Cook
  1 sibling, 2 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 21:19 UTC (permalink / raw)
  To: Emrah Demir
  Cc: Linux Kernel Mailing List, Kees Cook, Dan Rosenberg,
	kernel-hardening, Dave Jones

On Wed, Apr 6, 2016 at 10:54 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I'd find a patch like the attached to be perfectly acceptable (in
> fact, we should have done this long ago).

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 21:19     ` Linus Torvalds
@ 2016-04-06 21:27       ` Kees Cook
  2016-04-06 21:32         ` Linus Torvalds
  2016-04-14  4:27       ` Kees Cook
  1 sibling, 1 reply; 22+ messages in thread
From: Kees Cook @ 2016-04-06 21:27 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Emrah Demir, Linux Kernel Mailing List, Dan Rosenberg,
	kernel-hardening, Dave Jones

On Wed, Apr 6, 2016 at 2:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Apr 6, 2016 at 10:54 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So I'd find a patch like the attached to be perfectly acceptable (in
>> fact, we should have done this long ago).
>
> 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.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 21:27       ` Kees Cook
@ 2016-04-06 21:32         ` Linus Torvalds
  0 siblings, 0 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-04-06 21:32 UTC (permalink / raw)
  To: Kees Cook
  Cc: Emrah Demir, Linux Kernel Mailing List, Dan Rosenberg,
	kernel-hardening, Dave Jones

On Wed, Apr 6, 2016 at 2:27 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Hrm, okay. I still think just changing the perms would be less
> troublesome.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-06 21:19     ` Linus Torvalds
  2016-04-06 21:27       ` Kees Cook
@ 2016-04-14  4:27       ` Kees Cook
  2016-04-14  7:39         ` Emrah Demir
  1 sibling, 1 reply; 22+ messages in thread
From: Kees Cook @ 2016-04-14  4:27 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Emrah Demir, Linux Kernel Mailing List, Dan Rosenberg,
	kernel-hardening, Dave Jones

On Wed, Apr 6, 2016 at 2:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Apr 6, 2016 at 10:54 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So I'd find a patch like the attached to be perfectly acceptable (in
>> fact, we should have done this long ago).
>
> 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.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file
  2016-04-14  4:27       ` Kees Cook
@ 2016-04-14  7:39         ` Emrah Demir
  0 siblings, 0 replies; 22+ messages in thread
From: Emrah Demir @ 2016-04-14  7:39 UTC (permalink / raw)
  To: Kees Cook
  Cc: Linus Torvalds, Linux Kernel Mailing List, Dan Rosenberg,
	kernel-hardening, Dave Jones, keescook

On 2016-04-14 00:27, Kees Cook wrote:
> On Wed, Apr 6, 2016 at 2:19 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Wed, Apr 6, 2016 at 10:54 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> 
>>> So I'd find a patch like the attached to be perfectly acceptable (in
>>> fact, we should have done this long ago).
>> 
>> 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.
> 
> 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?
> 

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2016-04-14  9:32 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-06 13:03 [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file Emrah Demir
2016-04-06 15:20 ` Linus Torvalds
2016-04-06 17:54   ` Linus Torvalds
2016-04-06 18:05     ` ed
2016-04-06 18:21       ` Kees Cook
2016-04-06 18:31       ` Linus Torvalds
2016-04-06 18:37         ` Kees Cook
2016-04-06 18:43           ` Linus Torvalds
2016-04-06 18:53             ` [kernel-hardening] " Yves-Alexis Perez
2016-04-06 19:02               ` Linus Torvalds
2016-04-06 19:11                 ` Yves-Alexis Perez
2016-04-06 19:19                   ` Borislav Petkov
2016-04-06 20:49                 ` Ingo Molnar
2016-04-06 19:23             ` Bjørn Mork
2016-04-06 18:52       ` Christian Kujau
2016-04-06 18:53         ` Kees Cook
2016-04-06 21:19     ` Linus Torvalds
2016-04-06 21:27       ` Kees Cook
2016-04-06 21:32         ` Linus Torvalds
2016-04-14  4:27       ` Kees Cook
2016-04-14  7:39         ` Emrah Demir
2016-04-06 18:03 ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).