Removal of the kernel code/data/bss resources does break kexec/kdump

Removal of the kernel code/data/bss resources does break kexec/kdump

[Freeman Zhang]

Mr. Torvalds,

I do notice your recent commit:

> commit c4004b02f8e5b9ce357a0bb1641756cc86962664
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Apr 6 13:45:07 2016 -0700
>
>     x86: remove the kernel code/data/bss resources from /proc/iomem
>
> Let's see if anybody even notices.  I doubt anybody uses this, and it
> does expose addresses that should be randomized, so let's just remove
> the code.  It's old and traditional, and it used to be cute, but we
> should have removed this long ago.
>
> If it turns out anybody notices and this breaks something, we'll have to
> revert this, and maybe we'll end up using other approaches instead
> (using %pK or similar).  But removing unnecessary code is always the
> preferred option.

Removal of these information causes 'kexec/kdump' to fail in the newer
kernel, as 'kexec/arch/i386/crashdump-x86.c' is coded this way:


/* Read kernel physical load addr from the file returned by proc_iomem()
 * (Kernel Code) and store in kexec_info */
static int get_kernel_paddr(struct kexec_info *UNUSED(info),
                            struct crash_elf_info *elf_info)
{
               ...

      if (parse_iomem_single("Kernel code\n", &start, NULL) == 0) {
              elf_info->kern_paddr_start = start;
              dbgprintf("kernel load physical addr start = 0x%016Lx\n",
                        (unsigned long long)start);
              return 0;
      }

     fprintf(stderr, "Cannot determine kernel physical load addr\n");
     return -1;
}


Should we revert this commit, or update kexec/kdump code?


Great respect!
Freeman

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Emrah Demir]

On 2016-04-14 06:14, Freeman Zhang wrote:
> Mr. Torvalds,
> 
> I do notice your recent commit:
> 
>> commit c4004b02f8e5b9ce357a0bb1641756cc86962664
>> Author: Linus Torvalds <torvalds@linux-foundation.org>
>> Date:   Wed Apr 6 13:45:07 2016 -0700
>> 
>>     x86: remove the kernel code/data/bss resources from /proc/iomem
>> 
>> Let's see if anybody even notices.  I doubt anybody uses this, and it
>> does expose addresses that should be randomized, so let's just remove
>> the code.  It's old and traditional, and it used to be cute, but we
>> should have removed this long ago.
>> 
>> If it turns out anybody notices and this breaks something, we'll have 
>> to
>> revert this, and maybe we'll end up using other approaches instead
>> (using %pK or similar).  But removing unnecessary code is always the
>> preferred option.
> 
> Removal of these information causes 'kexec/kdump' to fail in the newer
> kernel, as 'kexec/arch/i386/crashdump-x86.c' is coded this way:
> 
> 
> /* Read kernel physical load addr from the file returned by 
> proc_iomem()
>  * (Kernel Code) and store in kexec_info */
> static int get_kernel_paddr(struct kexec_info *UNUSED(info),
>                             struct crash_elf_info *elf_info)
> {
>                ...
> 
>       if (parse_iomem_single("Kernel code\n", &start, NULL) == 0) {
>               elf_info->kern_paddr_start = start;
>               dbgprintf("kernel load physical addr start = 0x%016Lx\n",
>                         (unsigned long long)start);
>               return 0;
>       }
> 
>      fprintf(stderr, "Cannot determine kernel physical load addr\n");
>      return -1;
> }
> 
> 
> Should we revert this commit, or update kexec/kdump code?
> 

Ubuntu also has some issues with this patch. I think there are several 
issue that hasn't been noticed.
It would be better you to revert this patch. Updating kexec/kdump might 
not solve this problem.

Kees Cook proposed to write a %pK formatted patch. This would solve most 
of the problems.

https://lkml.org/lkml/2016/4/14/18

Best regards!
Emrah Demir

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Linus Torvalds]

On Thu, Apr 14, 2016 at 4:07 AM, Emrah Demir <ed@abdsec.com> wrote:
>
> Kees Cook proposed to write a %pK formatted patch. This would solve most of
> the problems.

Actually, %pK is horrible in /proc and /sys files, and does the wrong thing.

It uses the current creds for deciding what to do, which is exactly
the wrong thing (for all the usual reasons) for a file access from a
security standpoint. Sadly, almost every use of %pK gets this wrong.
Thankfully, it's much less of a problem for reads than for writes, but
it's still wrong.

A file access should use "file->f_cred", but the seq_file interface
sadly doesn't expose any way to do that.

I'll take a look, but it's non-trivial to get right. %pK turns out to
have been seriously mis-designed, and is basically almost always a
bug.

          Linus

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Emrah Demir]

On 2016-04-14 13:40, Linus Torvalds wrote:
> 
> Actually, %pK is horrible in /proc and /sys files, and does the wrong 
> thing.
> 

I agree with that, but for now there is no way to make things right in 
/proc or /sys.


> 
> A file access should use "file->f_cred", but the seq_file interface
> sadly doesn't expose any way to do that.
> 
> I'll take a look, but it's non-trivial to get right. %pK turns out to
> have been seriously mis-designed, and is basically almost always a
> bug.
> 
>           Linus

In another way, maybe it's good to remove code dependencies on /proc 
sensitive files like /proc/iomem.

Kees Coook: "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)"

Freeman Zhang: "Removal of these information causes 'kexec/kdump' to 
fail in the newer
kernel"

Removing such dependencies would make things better and code/bss/data 
sections could be removed.



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Linus Torvalds]

On Thu, Apr 14, 2016 at 1:27 PM, Emrah Demir <ed@abdsec.com> wrote:
> On 2016-04-14 13:40, Linus Torvalds wrote:
>>
>>
>> Actually, %pK is horrible in /proc and /sys files, and does the wrong
>> thing.
>
> I agree with that, but for now there is no way to make things right in /proc
> or /sys.

Well, there is now.

I've pushed out my attempt at fixing things properly. Please check
that kexec works - and if kexec ends up reading that file as non-root,
I don't know what to say/do.

Here's the three relevant cases:

   cat /proc/iomem
   sudo cat /proc/iomem
   sudo cat < /proc/iomem

and two of them will now show the resource ranges as just plain
zeroes. But yes, it needed extra infrastructure to be able to get this
right.

Note that while %pK is always wrong in /proc and /sys files, in this
case it would have been particularly wrong, since the values can be
64-bit even on a 32-bit architecture, so trying to show them as
pointers would have gotten not just the capability handling wrong, it
would have truncated a 64-bit value to 32 bits in that case.

                       Linus

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Kees Cook]

On Thu, Apr 14, 2016 at 6:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Apr 14, 2016 at 1:27 PM, Emrah Demir <ed@abdsec.com> wrote:
>> On 2016-04-14 13:40, Linus Torvalds wrote:
>>>
>>>
>>> Actually, %pK is horrible in /proc and /sys files, and does the wrong
>>> thing.
>>
>> I agree with that, but for now there is no way to make things right in /proc
>> or /sys.
>
> Well, there is now.
>
> I've pushed out my attempt at fixing things properly. Please check
> that kexec works - and if kexec ends up reading that file as non-root,
> I don't know what to say/do.
>
> Here's the three relevant cases:
>
>    cat /proc/iomem
>    sudo cat /proc/iomem
>    sudo cat < /proc/iomem
>
> and two of them will now show the resource ranges as just plain
> zeroes. But yes, it needed extra infrastructure to be able to get this
> right.
>
> Note that while %pK is always wrong in /proc and /sys files, in this
> case it would have been particularly wrong, since the values can be
> 64-bit even on a 32-bit architecture, so trying to show them as
> pointers would have gotten not just the capability handling wrong, it
> would have truncated a 64-bit value to 32 bits in that case.

Yup, that's why I was saying I was going to try to cook something up
for -next. It isn't a trivial change. :) Thanks for fixing it up!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Emrah Demir]

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

On 2016-04-15 00:41, Kees Cook wrote:
> On Thu, Apr 14, 2016 at 6:02 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Apr 14, 2016 at 1:27 PM, Emrah Demir <ed@abdsec.com> wrote:
>>> On 2016-04-14 13:40, Linus Torvalds wrote:

>> I've pushed out my attempt at fixing things properly. Please check
>> that kexec works - and if kexec ends up reading that file as non-root,
>> I don't know what to say/do.
>> 
>> Here's the three relevant cases:
>> 
>>    cat /proc/iomem
>>    sudo cat /proc/iomem
>>    sudo cat < /proc/iomem
>> 
>> and two of them will now show the resource ranges as just plain
>> zeroes. But yes, it needed extra infrastructure to be able to get this
>> right.
>> 

> 
> Yup, that's why I was saying I was going to try to cook something up
> for -next. It isn't a trivial change. :) Thanks for fixing it up!
> 

file_ns_capable bring some problems. I used capable and now there is no 
problem as far as I tested.

It'is attached.

Note: I couldn't write "Noted-by:, Reported-by:" Could you write them

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-proc-iomem-only-expose-physical-resource-addresses-t.patch --]
[-- Type: text/x-diff; name=0001-proc-iomem-only-expose-physical-resource-addresses-t.patch, Size: 1918 bytes --]

From 3f17bf8b1e9e129cdaf478a9e83a648b27f6fe73 Mon Sep 17 00:00:00 2001
From: Emrah Demir <ed@abdsec.com>
Date: Fri, 15 Apr 2016 18:34:12 +0300
Subject: [PATCH] /proc/iomem: only expose physical resource addresses to top
 user fixing capability
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In commit 51d7b120418e99d6b3bf8df9eb3cc31e8171dee4

file_ns_capable used in order to show value as 0, but while building
some problems coming up.

kernel/resource.c: In function ‘r_show’:
kernel/resource.c:116:23: error: ‘struct seq_file’ has no member named ‘file’
  if (file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN)) {
                       ^
scripts/Makefile.build:291: recipe for target 'kernel/resource.o' failed
make[1]: *** [kernel/resource.o] Error 1

so instead of file_ns_capable using capable solves problem.

It's now working.

ltr@reces-0:~$ cat /proc/iomem
00000000-00000000 : reserved
00000000-00000000 : System RAM
00000000-00000000 : reserved
00000000-00000000 : PCI Bus 0000:00
00000000-00000000 : Video ROM
00000000-00000000 : Adapter ROM
00000000-00000000 : reserved

root@reces-0:/home/ltr# cat /proc/iomem
00000000-00000fff : reserved
00001000-0009dbff : System RAM
0009dc00-0009ffff : reserved
000a0000-000bffff : PCI Bus 0000:00
000c0000-000cedff : Video ROM
000cf000-000cffff : Adapter ROM
000e0000-000fffff : reserved

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

diff --git a/kernel/resource.c b/kernel/resource.c
index c7727db..52a27e3 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -113,7 +113,7 @@ static int r_show(struct seq_file *m, void *v)
 		if (p->parent == root)
 			break;
 
-	if (file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN)) {
+	if (capable(CAP_SYS_ADMIN)) {
 		start = r->start;
 		end = r->end;
 	} else {
-- 
2.8.0.rc3


[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Linus Torvalds]

On Fri, Apr 15, 2016 at 8:46 AM, Emrah Demir <ed@abdsec.com> wrote:
>
> file_ns_capable bring some problems.

No it does not. file_ns_capable() is _required_ for security. We have
had several security issues with file IO doing "capable()", and it's
wrong and insecure.

> I used capable and now there is no problem as far as I tested.

You just screwed up the security, and with your change, a suid
application can be fooled into making the hidden data available to
non-secure users.

"capable()" is wrong. For file reading,  you *have* to use
file_ns_capable(). It really is that simple. You should not test the
capabilities of the process, you should be testing the capabilities of
the file descriptor, which comes from the *open-time* capabilities.

It sounds like you applied just the patch to kernel/resource.c,
without applying the infrastructure patch.

You also need commit 34dbbcdbf633 ("Make file credentials available to
the seqfile interfaces").

          Linus

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Emrah Demir]

On 2016-04-15 12:48, Linus Torvalds wrote:
> On Fri, Apr 15, 2016 at 8:46 AM, Emrah Demir <ed@abdsec.com> wrote:
>> 
>> file_ns_capable bring some problems.
> 
> No it does not. file_ns_capable() is _required_ for security. We have
> had several security issues with file IO doing "capable()", and it's
> wrong and insecure.

Of course file_ns_capable() is required, I didn't know you made some 
changes in include/linux/seq_file.h file

>> I used capable and now there is no problem as far as I tested.
> 
> You just screwed up the security, and with your change, a suid
> application can be fooled into making the hidden data available to
> non-secure users.
> 

Sorry for screwing up the security. I would never wish to do that. As 
you said a suid application could screw up things.

> "capable()" is wrong. For file reading,  you *have* to use
> file_ns_capable(). It really is that simple. You should not test the
> capabilities of the process, you should be testing the capabilities of
> the file descriptor, which comes from the *open-time* capabilities.
> 
> It sounds like you applied just the patch to kernel/resource.c,
> without applying the infrastructure patch.
> 
> You also need commit 34dbbcdbf633 ("Make file credentials available to
> the seqfile interfaces").
> 

Yeah, you are right. I didn't see that commit. It's okay now. Thank you!

-Emrah


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Dave Young]

On 04/14/16 at 06:02pm, Linus Torvalds wrote:
> On Thu, Apr 14, 2016 at 1:27 PM, Emrah Demir <ed@abdsec.com> wrote:
> > On 2016-04-14 13:40, Linus Torvalds wrote:
> >>
> >>
> >> Actually, %pK is horrible in /proc and /sys files, and does the wrong
> >> thing.
> >
> > I agree with that, but for now there is no way to make things right in /proc
> > or /sys.
> 
> Well, there is now.
> 
> I've pushed out my attempt at fixing things properly. Please check
> that kexec works - and if kexec ends up reading that file as non-root,
> I don't know what to say/do.
> 

Checked kexec/kdump, for kdump it will read /proc/kcore, /proc/kcore is root
only thus it will be fine with the changes.

Though in the comment of kexec_load syscall it says "for obvious reasons only
root may call it", but in the code it is using:
        if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
                return -EPERM;

So for kexec CAP_SYS_BOOT is enough, not necessarily to be root.

It is not clear how to handle it, maybe we can assume nobody is using it as
non-root, leave it as is or just add |CAP_SYS_BOOT for /proc/iomem?

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Linus Torvalds]

On Tue, Apr 19, 2016 at 2:04 AM, Dave Young <dyoung@redhat.com> wrote:
>
> It is not clear how to handle it, maybe we can assume nobody is using it as
> non-root, leave it as is or just add |CAP_SYS_BOOT for /proc/iomem?

Pretty much nobody uses fine-grained capabilities anyway - they are
one of those bad security things that generally add more complexity
than value(*) - so I wouldn't worry about it unless you actually find
something that cares.

             Linus

(*) The one exception tends to be certain network services that can
use CAP_NET_BIND_SERVICE like things to really lower their attack
surface. But certainly not one-time things like kexec.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Dave Young]

On 04/19/16 at 09:20am, Linus Torvalds wrote:
> On Tue, Apr 19, 2016 at 2:04 AM, Dave Young <dyoung@redhat.com> wrote:
> >
> > It is not clear how to handle it, maybe we can assume nobody is using it as
> > non-root, leave it as is or just add |CAP_SYS_BOOT for /proc/iomem?
> 
> Pretty much nobody uses fine-grained capabilities anyway - they are
> one of those bad security things that generally add more complexity
> than value(*) - so I wouldn't worry about it unless you actually find
> something that cares.

Agreed that leaving it as is should be fine according to you said about
fine-grained capabilities usage.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Removal of the kernel code/data/bss resources does break kexec/kdump

[Baoquan He]

On 04/14/16 at 06:14pm, Freeman Zhang wrote:
> Mr. Torvalds,
> 
> I do notice your recent commit:
> 
> > commit c4004b02f8e5b9ce357a0bb1641756cc86962664
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Wed Apr 6 13:45:07 2016 -0700
> >
> >     x86: remove the kernel code/data/bss resources from /proc/iomem
> >
> > Let's see if anybody even notices.  I doubt anybody uses this, and it
> > does expose addresses that should be randomized, so let's just remove
> > the code.  It's old and traditional, and it used to be cute, but we
> > should have removed this long ago.
> >
> > If it turns out anybody notices and this breaks something, we'll have to
> > revert this, and maybe we'll end up using other approaches instead
> > (using %pK or similar).  But removing unnecessary code is always the
> > preferred option.
> 
> Removal of these information causes 'kexec/kdump' to fail in the newer
> kernel, as 'kexec/arch/i386/crashdump-x86.c' is coded this way:

Yes, kdump need this to construct the program header of elf file for
kernel text because users want kernel text included in /proc/vmcore. Now
kexec utility relies on /proc/iomem to get the phisical addr of  _text.
Without /proc/iomem we may need to pass it out or take other ways.

> 
> 
> /* Read kernel physical load addr from the file returned by proc_iomem()
>  * (Kernel Code) and store in kexec_info */
> static int get_kernel_paddr(struct kexec_info *UNUSED(info),
>                             struct crash_elf_info *elf_info)
> {
>                ...
> 
>       if (parse_iomem_single("Kernel code\n", &start, NULL) == 0) {
>               elf_info->kern_paddr_start = start;
>               dbgprintf("kernel load physical addr start = 0x%016Lx\n",
>                         (unsigned long long)start);
>               return 0;
>       }
> 
>      fprintf(stderr, "Cannot determine kernel physical load addr\n");
>      return -1;
> }
> 
> 
> Should we revert this commit, or update kexec/kdump code?
> 
> 
> Great respect!
> Freeman
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec