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

DISCUSSION Freeman Zhang 6 participants 13 messages Apr 14, 2016 – Apr 20, 2016

Subsystems (2)

Live update kexec handover (KHO)

lore

Share & tools

lore

Freeman Zhang

author

2016-04-14 10:15 UTC #0 lore raw

Mr. Torvalds,

I do notice your recent commit:

Quoted 11 lines

commit c4004b02f8e5b9ce357a0bb1641756cc86962664
Author: Linus Torvalds <[email protected]>
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.

Quoted 4 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Emrah Demir

2016-04-14 11:09 UTC #1 lore raw

Quoted 43 lines · Freeman Zhang

Mr. Torvalds,
I do notice your recent commit:
Quoted 11 lines
commit c4004b02f8e5b9ce357a0bb1641756cc86962664
Author: Linus Torvalds <[email protected]>
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.
Quoted 4 lines
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;
}

Quoted 2 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Baoquan He

2016-04-14 11:27 UTC #12 lore raw

In reply to Freeman Zhang

Quoted 15 lines · Freeman Zhang

Mr. Torvalds,
I do notice your recent commit:
Quoted 11 lines
commit c4004b02f8e5b9ce357a0bb1641756cc86962664
Author: Linus Torvalds <[email protected]>
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 24 lines

/* 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?

Quoted 7 lines

Great respect!
Freeman
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Linus Torvalds

2016-04-14 17:41 UTC #2 lore raw

In reply to Emrah Demir

Quoted 3 lines · Emrah Demir

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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Emrah Demir

2016-04-14 20:28 UTC #3 lore raw

Quoted 3 lines · Linus Torvalds

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.

Quoted 9 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Linus Torvalds

2016-04-15 01:03 UTC #4 lore raw

Quoted 8 lines · Emrah Demir

Quoted 4 lines · Linus Torvalds
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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Kees Cook

2016-04-15 04:42 UTC #5 lore raw

Quoted 26 lines · Linus Torvalds

Quoted 8 lines · Emrah Demir
Quoted 4 lines · Linus Torvalds
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.

Quoted 5 lines

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

Signature · 7 lines

Kees Cook
Chrome OS & Brillo Security

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Emrah Demir

2016-04-15 15:48 UTC #6 lore raw

Quoted 15 lines · Kees Cook

Quoted 13 lines · Linus Torvalds
Quoted 1 line · Emrah Demir
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

Quoted 9 lines

Quoted 4 lines
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

From 3f17bf8b1e9e129cdaf478a9e83a648b27f6fe73 Mon Sep 17 00:00:00 2001
From: Emrah Demir <[email protected]>
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 <[email protected]>

---
 kernel/resource.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/resource.c +1 -1

index c7727db..52a27e3 100644
@@ -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 {

Signature · 1 line

2.8.0.rc3

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Linus Torvalds

2016-04-15 16:49 UTC #7 lore raw

Quoted 2 lines · Emrah Demir

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.

Quoted 1 line

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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Emrah Demir

2016-04-15 17:09 UTC #8 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 2 lines · Emrah Demir
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

Quoted 6 lines

Quoted 1 line
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.

Quoted 11 lines

"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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Dave Young

2016-04-19 09:04 UTC #9 lore raw

In reply to Linus Torvalds

Quoted 7 lines · Linus Torvalds

Quoted 6 lines · Emrah Demir
Quoted 4 lines · Linus Torvalds
Actually, %pK is horrible in /proc and /sys files, and does the wrong thing.

Quoted 9 lines

Quoted 2 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Linus Torvalds

2016-04-19 16:20 UTC #10 lore raw

Quoted 3 lines · Dave Young

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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Dave Young

2016-04-20 01:13 UTC #11 lore raw

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · Dave Young
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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec