[PATCH] kernel/time: Make /proc/timer_list mode 0400

[PATCH] kernel/time: Make /proc/timer_list mode 0400

[Marcus Meissner]

Hi,

/proc/timer_list contains kernel addresses, like e.g.:
 #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
 ...

Avoid leaking them to user space to make writing kernel exploits a bit harder.

(I currently cannot think of a userland tool that uses this, this is
likely pretty much root-only.)

Ciao, Marcus

Signed-off-by: Marcus Meissner <meissner@suse.de>
---
 kernel/time/timer_list.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index ab8f5e3..5ae1ce3 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -293,7 +293,7 @@ static int __init init_timer_list_procfs(void)
 {
 	struct proc_dir_entry *pe;
 
-	pe = proc_create("timer_list", 0444, NULL, &timer_list_fops);
+	pe = proc_create("timer_list", 0400, NULL, &timer_list_fops);
 	if (!pe)
 		return -ENOMEM;
 	return 0;
-- 
1.7.1

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Peter Zijlstra]

On Wed, 2010-11-17 at 18:08 +0100, Marcus Meissner wrote:
> Hi,
> 
> /proc/timer_list contains kernel addresses, like e.g.:
>  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
>  ...
> 
> Avoid leaking them to user space to make writing kernel exploits a bit harder.
> 
> (I currently cannot think of a userland tool that uses this, this is
> likely pretty much root-only.)

iirc powertop parses this..

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Marcus Meissner]

On Wed, Nov 17, 2010 at 06:18:32PM +0100, Peter Zijlstra wrote:
> On Wed, 2010-11-17 at 18:08 +0100, Marcus Meissner wrote:
> > Hi,
> > 
> > /proc/timer_list contains kernel addresses, like e.g.:
> >  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
> >  ...
> > 
> > Avoid leaking them to user space to make writing kernel exploits a bit harder.
> > 
> > (I currently cannot think of a userland tool that uses this, this is
> > likely pretty much root-only.)
> 
> iirc powertop parses this..

powertop already says on startup:

	PowerTOP needs to be run as root to collect enough information

And as developer tool it usually is for people having root access.

Ciao, Marcus

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Peter Zijlstra]

On Wed, 2010-11-17 at 18:21 +0100, Marcus Meissner wrote:
> On Wed, Nov 17, 2010 at 06:18:32PM +0100, Peter Zijlstra wrote:
> > On Wed, 2010-11-17 at 18:08 +0100, Marcus Meissner wrote:
> > > Hi,
> > > 
> > > /proc/timer_list contains kernel addresses, like e.g.:
> > >  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
> > >  ...
> > > 
> > > Avoid leaking them to user space to make writing kernel exploits a bit harder.
> > > 
> > > (I currently cannot think of a userland tool that uses this, this is
> > > likely pretty much root-only.)
> > 
> > iirc powertop parses this..
> 
> powertop already says on startup:
> 
> 	PowerTOP needs to be run as root to collect enough information

I know, but you said you didn't know of a tool that used the file.. now
you do ;-)

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Pavel Machek]

Hi!

> > > /proc/timer_list contains kernel addresses, like e.g.:
> > >  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
> > >  ...
> > > 
> > > Avoid leaking them to user space to make writing kernel exploits a bit harder.
> > > 
> > > (I currently cannot think of a userland tool that uses this, this is
> > > likely pretty much root-only.)
> > 
> > iirc powertop parses this..
> 
> powertop already says on startup:
> 
> 	PowerTOP needs to be run as root to collect enough information
> 
> And as developer tool it usually is for people having root access.

.....and now you are actively decreasing security.

Yes, developers usally have root access, but no, developers do not
like to work as root.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Andrew Morton]

On Wed, 17 Nov 2010 18:18:32 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Wed, 2010-11-17 at 18:08 +0100, Marcus Meissner wrote:
> > Hi,
> > 
> > /proc/timer_list contains kernel addresses, like e.g.:
> >  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
> >  ...
> > 
> > Avoid leaking them to user space to make writing kernel exploits a bit harder.
> > 
> > (I currently cannot think of a userland tool that uses this, this is
> > likely pretty much root-only.)
> 
> iirc powertop parses this..

I bet it doesn't look at the kernel address (why was that added in the
first place, anyway?)

I'd suggest that the risk of breakage would be much less if we left the
file permissions alone and arranged for those addresses to be
0000000000000000 for non-root readers.

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Thomas Gleixner]

On Wed, 17 Nov 2010, Andrew Morton wrote:

> On Wed, 17 Nov 2010 18:18:32 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Wed, 2010-11-17 at 18:08 +0100, Marcus Meissner wrote:
> > > Hi,
> > > 
> > > /proc/timer_list contains kernel addresses, like e.g.:
> > >  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
> > >  ...
> > > 
> > > Avoid leaking them to user space to make writing kernel exploits a bit harder.
> > > 
> > > (I currently cannot think of a userland tool that uses this, this is
> > > likely pretty much root-only.)
> > 
> > iirc powertop parses this..
> 
> I bet it doesn't look at the kernel address (why was that added in the
> first place, anyway?)
> 
> I'd suggest that the risk of breakage would be much less if we left the
> file permissions alone and arranged for those addresses to be
> 0000000000000000 for non-root readers.

You beat me to it. Having the full information is quite helpful at
times.

Thanks,

	tglx

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 17 Nov 2010, Andrew Morton wrote:
> 
> > On Wed, 17 Nov 2010 18:18:32 +0100
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > > On Wed, 2010-11-17 at 18:08 +0100, Marcus Meissner wrote:
> > > > Hi,
> > > > 
> > > > /proc/timer_list contains kernel addresses, like e.g.:
> > > >  #0: <c000000001404158>, tick_sched_timer, S:01, .tick_nohz_restart_sched_tick, swapper/0
> > > >  ...
> > > > 
> > > > Avoid leaking them to user space to make writing kernel exploits a bit harder.
> > > > 
> > > > (I currently cannot think of a userland tool that uses this, this is
> > > > likely pretty much root-only.)
> > > 
> > > iirc powertop parses this..
> > 
> > I bet it doesn't look at the kernel address (why was that added in the
> > first place, anyway?)
> > 
> > I'd suggest that the risk of breakage would be much less if we left the
> > file permissions alone and arranged for those addresses to be
> > 0000000000000000 for non-root readers.
> 
> You beat me to it. Having the full information is quite helpful at
> times.

We should do something like:

	if (!capable(CAP_SYS_ADMIN))
		print_ptr = NULL;

	sprintf(s, "%p", print_ptr);

Thanks,

	Ingo

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Eric Dumazet]

Le jeudi 18 novembre 2010 à 09:12 +0100, Ingo Molnar a écrit :

> We should do something like:
> 
> 	if (!capable(CAP_SYS_ADMIN))
> 		print_ptr = NULL;
> 
> 	sprintf(s, "%p", print_ptr);

A while ago, Thomas suggested following idea :

<quote> 

Considering the amount of duplication you are about to do, you may
want to think about adding a new pointer format extension then. We
already have special '%p' modes for IPv6 addresse, MAC addresses and
various other pointer types. It wouldn't be hard to add one which
does not print (null).

</quote>

http://www.spinics.net/lists/netdev/msg146606.html

So the if (!capable(CAP_SYS_ADMIN)) could be centralized in %pX handling
(X being a new letter, not actually 'X')

Re: [PATCH] kernel/time: Make /proc/timer_list mode 0400

[Pavel Machek]

Hi!

> > > > iirc powertop parses this..
> > > 
> > > I bet it doesn't look at the kernel address (why was that added in the
> > > first place, anyway?)
> > > 
> > > I'd suggest that the risk of breakage would be much less if we left the
> > > file permissions alone and arranged for those addresses to be
> > > 0000000000000000 for non-root readers.
> > 
> > You beat me to it. Having the full information is quite helpful at
> > times.
> 
> We should do something like:
> 
> 	if (!capable(CAP_SYS_ADMIN))
> 		print_ptr = NULL;
> 
> 	sprintf(s, "%p", print_ptr);

Having single file that contains different stuff based on capable() is
very very ugly.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html