[stable] Removing or restricting timer_stats

[stable] Removing or restricting timer_stats

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 559 bytes --]

The timer_stats feature was removed upstream by:

commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
Author: Kees Cook <keescook@chromium.org>
Date:   Wed Feb 8 11:26:59 2017 -0800

    time: Remove CONFIG_TIMER_STATS

I'm hesitant to propose removing a feature in stable, even if it is
redundant.  What I've done for Debian stable is to restrict it to the
initial pid namespace (see attached).  Would that be a reasonable
alternative change for stable branches?

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.


[-- Attachment #1.2: timer-restrict-timer_stats-to-initial-pid-namespace.patch --]
[-- Type: text/x-patch, Size: 1182 bytes --]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 13 Mar 2017 23:03:29 +0000
Subject: timer: Restrict timer_stats to initial PID namespace
Bug-Debian-Security: https://security-tracker.debian.org/tracker/CVE-2017-5967

The timer_stats facility should filter and translate PIDs if opened
from a non-initial PID namespace, to avoid leaking information about
the wider system.  Unfortunately it has now been removed upstream (as
redundant) instead of being fixed.  For stable, fix the leak by only
allowing access from the initial PID namespace.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -42,6 +42,7 @@
 #include <linux/sched.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/pid_namespace.h>
 
 #include <asm/uaccess.h>
 
@@ -394,6 +395,13 @@ static ssize_t tstats_write(struct file
 
 static int tstats_open(struct inode *inode, struct file *filp)
 {
+	/*
+	 * We don't filter PIDs, so must only allow access from initial
+	 * PID namespace.
+	 */
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		return -EPERM;
+
 	return single_open(filp, tstats_show, NULL);
 }
 

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

Re: [stable] Removing or restricting timer_stats

[Kees Cook]

On Mon, Apr 17, 2017 at 7:25 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> The timer_stats feature was removed upstream by:
>
> commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
> Author: Kees Cook <keescook@chromium.org>
> Date:   Wed Feb 8 11:26:59 2017 -0800
>
>     time: Remove CONFIG_TIMER_STATS
>
> I'm hesitant to propose removing a feature in stable, even if it is
> redundant.  What I've done for Debian stable is to restrict it to the
> initial pid namespace (see attached).  Would that be a reasonable
> alternative change for stable branches?

Seems like a reasonable approach. The only stuff that should need this
should only be running in the init_ns anyway...

-Kees

-- 
Kees Cook
Pixel Security

Re: [stable] Removing or restricting timer_stats

[Greg KH]

On Tue, Apr 18, 2017 at 03:25:05AM +0100, Ben Hutchings wrote:
> The timer_stats feature was removed upstream by:
> 
> commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
> Author: Kees Cook <keescook@chromium.org>
> Date:���Wed Feb 8 11:26:59 2017 -0800
> 
> ����time: Remove CONFIG_TIMER_STATS
> 
> I'm hesitant to propose removing a feature in stable, even if it is
> redundant.  What I've done for Debian stable is to restrict it to the
> initial pid namespace (see attached).  Would that be a reasonable
> alternative change for stable branches?

I don't mind removing things in stable as that's what happened in
Linus's tree, and it was removed for a reason.  We've done it before,
and I'm more hesitant to apply something that works a bit
"differently".

Kees, any objection for me just taking the "full" patch in the stable
kernels?

thanks,

greg k-h

Re: [stable] Removing or restricting timer_stats

[Kees Cook]

On Wed, Apr 19, 2017 at 4:50 AM, Greg KH <greg@kroah.com> wrote:
> On Tue, Apr 18, 2017 at 03:25:05AM +0100, Ben Hutchings wrote:
>> The timer_stats feature was removed upstream by:
>>
>> commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
>> Author: Kees Cook <keescook@chromium.org>
>> Date:   Wed Feb 8 11:26:59 2017 -0800
>>
>>     time: Remove CONFIG_TIMER_STATS
>>
>> I'm hesitant to propose removing a feature in stable, even if it is
>> redundant.  What I've done for Debian stable is to restrict it to the
>> initial pid namespace (see attached).  Would that be a reasonable
>> alternative change for stable branches?
>
> I don't mind removing things in stable as that's what happened in
> Linus's tree, and it was removed for a reason.  We've done it before,
> and I'm more hesitant to apply something that works a bit
> "differently".
>
> Kees, any objection for me just taking the "full" patch in the stable
> kernels?

Arjan said it would break powertop, IIRC.

-Kees

-- 
Kees Cook
Pixel Security

Re: [stable] Removing or restricting timer_stats

[Greg KH]

On Wed, Apr 19, 2017 at 07:54:34AM -0700, Kees Cook wrote:
> On Wed, Apr 19, 2017 at 4:50 AM, Greg KH <greg@kroah.com> wrote:
> > On Tue, Apr 18, 2017 at 03:25:05AM +0100, Ben Hutchings wrote:
> >> The timer_stats feature was removed upstream by:
> >>
> >> commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
> >> Author: Kees Cook <keescook@chromium.org>
> >> Date:   Wed Feb 8 11:26:59 2017 -0800
> >>
> >>     time: Remove CONFIG_TIMER_STATS
> >>
> >> I'm hesitant to propose removing a feature in stable, even if it is
> >> redundant.  What I've done for Debian stable is to restrict it to the
> >> initial pid namespace (see attached).  Would that be a reasonable
> >> alternative change for stable branches?
> >
> > I don't mind removing things in stable as that's what happened in
> > Linus's tree, and it was removed for a reason.  We've done it before,
> > and I'm more hesitant to apply something that works a bit
> > "differently".
> >
> > Kees, any objection for me just taking the "full" patch in the stable
> > kernels?
> 
> Arjan said it would break powertop, IIRC.

So we broke it in 4.11?  That doesn't seem to sound reasonable, how are
you getting away with that?  :)

thanks,

greg k-h

Re: [stable] Removing or restricting timer_stats

[Kees Cook]

On Wed, Apr 19, 2017 at 7:57 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Apr 19, 2017 at 07:54:34AM -0700, Kees Cook wrote:
>> On Wed, Apr 19, 2017 at 4:50 AM, Greg KH <greg@kroah.com> wrote:
>> > On Tue, Apr 18, 2017 at 03:25:05AM +0100, Ben Hutchings wrote:
>> >> The timer_stats feature was removed upstream by:
>> >>
>> >> commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
>> >> Author: Kees Cook <keescook@chromium.org>
>> >> Date:   Wed Feb 8 11:26:59 2017 -0800
>> >>
>> >>     time: Remove CONFIG_TIMER_STATS
>> >>
>> >> I'm hesitant to propose removing a feature in stable, even if it is
>> >> redundant.  What I've done for Debian stable is to restrict it to the
>> >> initial pid namespace (see attached).  Would that be a reasonable
>> >> alternative change for stable branches?
>> >
>> > I don't mind removing things in stable as that's what happened in
>> > Linus's tree, and it was removed for a reason.  We've done it before,
>> > and I'm more hesitant to apply something that works a bit
>> > "differently".
>> >
>> > Kees, any objection for me just taking the "full" patch in the stable
>> > kernels?
>>
>> Arjan said it would break powertop, IIRC.
>
> So we broke it in 4.11?  That doesn't seem to sound reasonable, how are
> you getting away with that?  :)

tglx seemed to think that the same information was available elsewhere?

Thread was here: https://patchwork.kernel.org/patch/9561519/

-Kees

-- 
Kees Cook
Pixel Security

Re: [stable] Removing or restricting timer_stats

[Greg KH]

On Wed, Apr 19, 2017 at 08:01:18AM -0700, Kees Cook wrote:
> On Wed, Apr 19, 2017 at 7:57 AM, Greg KH <greg@kroah.com> wrote:
> > On Wed, Apr 19, 2017 at 07:54:34AM -0700, Kees Cook wrote:
> >> On Wed, Apr 19, 2017 at 4:50 AM, Greg KH <greg@kroah.com> wrote:
> >> > On Tue, Apr 18, 2017 at 03:25:05AM +0100, Ben Hutchings wrote:
> >> >> The timer_stats feature was removed upstream by:
> >> >>
> >> >> commit dfb4357da6ddbdf57d583ba64361c9d792b0e0b1
> >> >> Author: Kees Cook <keescook@chromium.org>
> >> >> Date:   Wed Feb 8 11:26:59 2017 -0800
> >> >>
> >> >>     time: Remove CONFIG_TIMER_STATS
> >> >>
> >> >> I'm hesitant to propose removing a feature in stable, even if it is
> >> >> redundant.  What I've done for Debian stable is to restrict it to the
> >> >> initial pid namespace (see attached).  Would that be a reasonable
> >> >> alternative change for stable branches?
> >> >
> >> > I don't mind removing things in stable as that's what happened in
> >> > Linus's tree, and it was removed for a reason.  We've done it before,
> >> > and I'm more hesitant to apply something that works a bit
> >> > "differently".
> >> >
> >> > Kees, any objection for me just taking the "full" patch in the stable
> >> > kernels?
> >>
> >> Arjan said it would break powertop, IIRC.
> >
> > So we broke it in 4.11?  That doesn't seem to sound reasonable, how are
> > you getting away with that?  :)
> 
> tglx seemed to think that the same information was available elsewhere?
> 
> Thread was here: https://patchwork.kernel.org/patch/9561519/

Getting the same info elsewhere is fine, breaking working tools is not.

I'm running powertop on 4.11-rc6 here and there doesn't seem to be
anything "broken".  And, as this is removed there, I don't see why
backporting it should matter.  Either it breaks someone, or it doesn't,
the kernel version shouldn't matter...

thanks,

greg k-h

Re: [stable] Removing or restricting timer_stats

[Arjan van de Ven]

On 4/19/2017 8:01 AM, Kees Cook wrote:

>>>
>>> Arjan said it would break powertop, IIRC.
>>
>> So we broke it in 4.11?  That doesn't seem to sound reasonable, how are
>> you getting away with that?  :)
>
> tglx seemed to think that the same information was available elsewhere?

tglx was working on patches to make the same information available elsewhere *in the future*.

unless you also backport that work (and wait for powertop to release/etc)...
just backporting the interface break without backporting the new interface... leaves things stuck.

Re: [stable] Removing or restricting timer_stats

[Greg KH]

On Wed, Apr 19, 2017 at 08:25:53AM -0700, Arjan van de Ven wrote:
> On 4/19/2017 8:01 AM, Kees Cook wrote:
> 
> > > > 
> > > > Arjan said it would break powertop, IIRC.
> > > 
> > > So we broke it in 4.11?  That doesn't seem to sound reasonable, how are
> > > you getting away with that?  :)
> > 
> > tglx seemed to think that the same information was available elsewhere?
> 
> tglx was working on patches to make the same information available elsewhere *in the future*.
> 
> unless you also backport that work (and wait for powertop to release/etc)...
> just backporting the interface break without backporting the new interface... leaves things stuck.

So, when 4.11 is released, things are then "stuck"?  How is taking
something that shows up in 4.11 and adding it to 4.10 going to break
anything "worse"?

I think this probably needs to be removed from 4.11 (i.e. added back),
right?

thanks,

greg k-h

Re: [stable] Removing or restricting timer_stats

[Kees Cook]

On Wed, Apr 19, 2017 at 8:37 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Apr 19, 2017 at 08:25:53AM -0700, Arjan van de Ven wrote:
>> On 4/19/2017 8:01 AM, Kees Cook wrote:
>>
>> > > >
>> > > > Arjan said it would break powertop, IIRC.
>> > >
>> > > So we broke it in 4.11?  That doesn't seem to sound reasonable, how are
>> > > you getting away with that?  :)
>> >
>> > tglx seemed to think that the same information was available elsewhere?
>>
>> tglx was working on patches to make the same information available elsewhere *in the future*.
>>
>> unless you also backport that work (and wait for powertop to release/etc)...
>> just backporting the interface break without backporting the new interface... leaves things stuck.
>
> So, when 4.11 is released, things are then "stuck"?  How is taking
> something that shows up in 4.11 and adding it to 4.10 going to break
> anything "worse"?
>
> I think this probably needs to be removed from 4.11 (i.e. added back),
> right?

I have no preference about this. tglx specifically asked for it to be
removed (rather than the pid-censoring I was suggesting).

-Kees

-- 
Kees Cook
Pixel Security