[PATCH] kernel: Restrict permissions of /proc/iomem.

[PATCH] kernel: Restrict permissions of /proc/iomem.

[zhanglin]

The permissions of /proc/iomem currently are -r--r--r--. Everyone can
see its content. As iomem contains information about the physical memory
content of the device, restrict the information only to root.

Signed-off-by: zhanglin <zhang.lin16@zte.com.cn>
---
 kernel/resource.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 30e1bc6..844456e 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -139,7 +139,8 @@ static int __init ioresources_init(void)
 {
 	proc_create_seq_data("ioports", 0, NULL, &resource_op,
 			&ioport_resource);
-	proc_create_seq_data("iomem", 0, NULL, &resource_op, &iomem_resource);
+	proc_create_seq_data("iomem", S_IRUSR, NULL, &resource_op,
+			&iomem_resource);
 	return 0;
 }
 __initcall(ioresources_init);
-- 
2.15.2

Re: [PATCH] kernel: Restrict permissions of /proc/iomem.

[Andrew Morton]

On Fri, 25 Oct 2019 16:56:41 +0800 zhanglin <zhang.lin16@zte.com.cn> wrote:

> The permissions of /proc/iomem currently are -r--r--r--. Everyone can
> see its content. As iomem contains information about the physical memory
> content of the device, restrict the information only to root.
> 
> ...
>
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -139,7 +139,8 @@ static int __init ioresources_init(void)
>  {
>  	proc_create_seq_data("ioports", 0, NULL, &resource_op,
>  			&ioport_resource);
> -	proc_create_seq_data("iomem", 0, NULL, &resource_op, &iomem_resource);
> +	proc_create_seq_data("iomem", S_IRUSR, NULL, &resource_op,
> +			&iomem_resource);
>  	return 0;
>  }
>  __initcall(ioresources_init);

It's risky to change things like this - heaven knows which userspace
applications might break.

Possibly we could obfuscate the information if that is considered
desirable.  Why is this a problem anyway?  What are the possible
exploit scenarios?

Can't the same info be obtained by running dmesg and looking at the
startup info?

Can't the user who is concerned about this run chmod 0400 /proc/iomem
at boot?

Maybe Kees has an opinion?

Re: [PATCH] kernel: Restrict permissions of /proc/iomem.

[Kees Cook]

On Fri, Oct 25, 2019 at 02:32:20PM -0700, Andrew Morton wrote:
> On Fri, 25 Oct 2019 16:56:41 +0800 zhanglin <zhang.lin16@zte.com.cn> wrote:
> 
> > The permissions of /proc/iomem currently are -r--r--r--. Everyone can
> > see its content. As iomem contains information about the physical memory
> > content of the device, restrict the information only to root.
> > 
> > ...
> >
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -139,7 +139,8 @@ static int __init ioresources_init(void)
> >  {
> >  	proc_create_seq_data("ioports", 0, NULL, &resource_op,
> >  			&ioport_resource);
> > -	proc_create_seq_data("iomem", 0, NULL, &resource_op, &iomem_resource);
> > +	proc_create_seq_data("iomem", S_IRUSR, NULL, &resource_op,
> > +			&iomem_resource);
> >  	return 0;
> >  }
> >  __initcall(ioresources_init);
> 
> It's risky to change things like this - heaven knows which userspace
> applications might break.
> 
> Possibly we could obfuscate the information if that is considered
> desirable.  Why is this a problem anyway?  What are the possible
> exploit scenarios?

This is already done: kptr_restrict sysctl already zeros these values
if it is set. e.g.:

00000000-00000000 : System RAM
  00000000-00000000 : Kernel code
  00000000-00000000 : Kernel data
  00000000-00000000 : Kernel bss

> Can't the same info be obtained by running dmesg and looking at the
> startup info?

Both virtual and physical address dumps in dmesg are considered "bad
form" these days and most have been removed.

> Can't the user who is concerned about this run chmod 0400 /proc/iomem
> at boot?

That is also possible.

> Maybe Kees has an opinion?

I do! :) System owners should either set kptr_restrict (or the CONFIG),
or do a chmod.

-- 
Kees Cook

Re: [PATCH] kernel: Restrict permissions of /proc/iomem.

[Christian Kujau]

On Mon, 28 Oct 2019, Kees Cook wrote:
> > It's risky to change things like this - heaven knows which userspace
> > applications might break.
> > 
> > Possibly we could obfuscate the information if that is considered
> > desirable.  Why is this a problem anyway?  What are the possible
> > exploit scenarios?
> 
> This is already done: kptr_restrict sysctl already zeros these values
> if it is set. e.g.:
> 
> 00000000-00000000 : System RAM
>   00000000-00000000 : Kernel code
>   00000000-00000000 : Kernel data
>   00000000-00000000 : Kernel bss
> 
> > Can't the same info be obtained by running dmesg and looking at the
> > startup info?
> 
> Both virtual and physical address dumps in dmesg are considered "bad
> form" these days and most have been removed.
> 
> > Can't the user who is concerned about this run chmod 0400 /proc/iomem
> > at boot?
> 
> That is also possible.

As a user, I still like this patch, or some variation of it. On various 
(server and desktop) systems I do this during boot for some time now and 
never had a problem:

find /proc -xdev -mindepth 1 -maxdepth 1 ! \( -name "[0-9]*" \
  -o -name cpuinfo -o -name modules -o -name loadavg -o -name meminfo \ 
  -o -name mounts -o -name net -o -name self -o -name diskstats \
  -o -name stat -o -name sys -o -name swaps -o -name thread-self \
  -o -name vmstat -o -name uptime \) -exec chmod -c go-rwx '{}' +

C.
-- 
BOFH excuse #436:

Daemon escaped from pentagram

Re: [PATCH] kernel: Restrict permissions of /proc/iomem.

[Dave Hansen]

On 10/25/19 1:56 AM, zhanglin wrote:
> The permissions of /proc/iomem currently are -r--r--r--. Everyone can
> see its content. As iomem contains information about the physical memory
> content of the device, restrict the information only to root.

For me, running as non-root on 5.4.0-rc4, I see:

$ 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 : pnp 00:00
00000000-00000000 : pnp 00:00
00000000-00000000 : pnp 00:00

All 0's since kernel/resource.c::r_show() does:

        if (file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN)) {
                start = r->start;
                end = r->end;
        } else {
                start = end = 0;
        }

Are you just looking at the file as root and assuming that users see the
same thing since they have read permissions?