[PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Mike Waychison]

It would be useful to build kernels with disabled support of /dev/port
for systems where we do not want to allow an administrator to access IO
ports directly.

To do so however, CONFIG_DEVPORT needs to be user-selectable.  Give this
configuration option a name and help description.

Google-Bug-Id: 3177114
Signed-off-by: Mike Waychison <mikew@google.com>
---
 drivers/char/Kconfig |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 49502bc..12503f1 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -593,10 +593,15 @@ config TELCLOCK
 	  controlling the behavior of this hardware.
 
 config DEVPORT
-	bool
+	bool "Support for /dev/port"
 	depends on !M68K
 	depends on ISA || PCI
 	default y
+	help
+	  Enabling this option allows for access to the /dev/port
+	  character device.  This device maps IO ports onto a file
+	  interface, so that users with sufficient privilege can read
+	  and write to IO ports directly.  If unsure, say yes.
 
 source "drivers/s390/char/Kconfig"
 
-- 
1.7.3.1

[PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

In some build environments, it is useful to allow disabling of IO
accesses to hardware, without having to rely on CAP_SYS_RAWIO (which is
already overloaded to mean many other things).  One way that userland
has access to IO accesses is via the iopl(2) and ioperm(2) system calls.

Allow disabling of these system calls from ever being available via a
configuration option, X86_SYS_IOPL.   This is implemented by simply
stubbing out the system calls and having them return ENOSYS when their
functionality is disabled.

Note that we default this option to 'y', so that existing kernel configs
will continue to support sys_iopl and sys_ioperm as before.

Google-Bug-Id: 3177114
Signed-off-by: Mike Waychison <mikew@google.com>
---
 arch/x86/Kconfig         |   12 ++++++++++++
 arch/x86/kernel/ioport.c |   12 ++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index da34972..295ae4d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1497,6 +1497,18 @@ config CC_STACKPROTECTOR
 	  detected and for those versions, this configuration option is
 	  ignored. (and a warning is printed during bootup)
 
+config X86_SYS_IOPL
+	bool "Enable use of sys_iopl and sys_ioperm system calls"
+	default y
+	---help---
+	  The sys_iopl and sys_ioperm allow applications that have the
+	  CAP_SYS_RAWIO to elevate their priviledge level so that they
+	  can perform IO accesses directly from userland.  This
+	  functionality is often used by userland drivers to drive
+	  hardware directly, bypassing the kernel.  Disabling this
+	  option may break certain hardware functions, so if in doubt,
+	  say yes.
+
 source kernel/Kconfig.hz
 
 config KEXEC
diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 8c96897..709db2f 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -17,6 +17,7 @@
 #include <linux/bitmap.h>
 #include <asm/syscalls.h>
 
+#ifdef CONFIG_X86_SYS_IOPL
 /*
  * this changes the io permissions bitmap in the current task.
  */
@@ -111,3 +112,14 @@ long sys_iopl(unsigned int level, struct pt_regs *regs)
 
 	return 0;
 }
+
+#else /* CONFIG_X86_SYS_IOPL */
+
+asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on) {
+	return -ENOSYS;
+}
+
+long sys_iopl(unsigned int level, struct pt_regs *regs) {
+	return -ENOSYS;
+}
+#endif /* CONFIG_X86_SYS_IOPL */
-- 
1.7.3.1

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[H. Peter Anvin]

On 07/14/2011 01:34 PM, Mike Waychison wrote:
> In some build environments, it is useful to allow disabling of IO
> accesses to hardware, without having to rely on CAP_SYS_RAWIO (which is
> already overloaded to mean many other things).  One way that userland
> has access to IO accesses is via the iopl(2) and ioperm(2) system calls.
> 
> Allow disabling of these system calls from ever being available via a
> configuration option, X86_SYS_IOPL.   This is implemented by simply
> stubbing out the system calls and having them return ENOSYS when their
> functionality is disabled.
> 
> Note that we default this option to 'y', so that existing kernel configs
> will continue to support sys_iopl and sys_ioperm as before.
> 

Wouldn't it be more useful for this to be a sysctl?  In particular, like
many similar things it probably should be a lockable sysctl (three
states: enabled, disabled, and locked-disabled).

Making it a compile-time option I'm very skeptical to.

	-hpa

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Thu, Jul 14, 2011 at 1:37 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/14/2011 01:34 PM, Mike Waychison wrote:
>> In some build environments, it is useful to allow disabling of IO
>> accesses to hardware, without having to rely on CAP_SYS_RAWIO (which is
>> already overloaded to mean many other things).  One way that userland
>> has access to IO accesses is via the iopl(2) and ioperm(2) system calls.
>>
>> Allow disabling of these system calls from ever being available via a
>> configuration option, X86_SYS_IOPL.   This is implemented by simply
>> stubbing out the system calls and having them return ENOSYS when their
>> functionality is disabled.
>>
>> Note that we default this option to 'y', so that existing kernel configs
>> will continue to support sys_iopl and sys_ioperm as before.
>>
>
> Wouldn't it be more useful for this to be a sysctl?  In particular, like
> many similar things it probably should be a lockable sysctl (three
> states: enabled, disabled, and locked-disabled).
>
> Making it a compile-time option I'm very skeptical to.

Are there existing examples of this already in the tree?

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[H. Peter Anvin]

On 07/14/2011 01:38 PM, Mike Waychison wrote:
>>
>> Wouldn't it be more useful for this to be a sysctl?  In particular, like
>> many similar things it probably should be a lockable sysctl (three
>> states: enabled, disabled, and locked-disabled).
>>
>> Making it a compile-time option I'm very skeptical to.
> 
> Are there existing examples of this already in the tree?
> 

I think so, but I don't know off the top of my head.

	-hpa

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Greg KH]

On Thu, Jul 14, 2011 at 01:34:52PM -0700, Mike Waychison wrote:
> It would be useful to build kernels with disabled support of /dev/port
> for systems where we do not want to allow an administrator to access IO
> ports directly.
> 
> To do so however, CONFIG_DEVPORT needs to be user-selectable.  Give this
> configuration option a name and help description.
> 
> Google-Bug-Id: 3177114

What is that field?  It's not something that I've ever seen before...

Please don't push internal fields like this to public patches, it only
causes confusion...

greg k-h

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Mike Waychison]

On Thu, Jul 14, 2011 at 2:21 PM, Greg KH <gregkh@suse.de> wrote:
> On Thu, Jul 14, 2011 at 01:34:52PM -0700, Mike Waychison wrote:
>> It would be useful to build kernels with disabled support of /dev/port
>> for systems where we do not want to allow an administrator to access IO
>> ports directly.
>>
>> To do so however, CONFIG_DEVPORT needs to be user-selectable.  Give this
>> configuration option a name and help description.
>>
>> Google-Bug-Id: 3177114
>
> What is that field?  It's not something that I've ever seen before...

This is for our internal reference.

>
> Please don't push internal fields like this to public patches, it only
> causes confusion...

It hasn't caused any problems so far.  It helps us track things as we
pull code back downstream.

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Andrew Morton]

On Thu, 14 Jul 2011 13:34:53 -0700
Mike Waychison <mikew@google.com> wrote:

> --- a/arch/x86/kernel/ioport.c
> +++ b/arch/x86/kernel/ioport.c
> @@ -17,6 +17,7 @@
>  #include <linux/bitmap.h>
>  #include <asm/syscalls.h>
>  
> +#ifdef CONFIG_X86_SYS_IOPL
>  /*
>   * this changes the io permissions bitmap in the current task.
>   */
> @@ -111,3 +112,14 @@ long sys_iopl(unsigned int level, struct pt_regs *regs)
>  
>  	return 0;
>  }
> +
> +#else /* CONFIG_X86_SYS_IOPL */
> +
> +asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on) {
> +	return -ENOSYS;
> +}
> +
> +long sys_iopl(unsigned int level, struct pt_regs *regs) {
> +	return -ENOSYS;
> +}
> +#endif /* CONFIG_X86_SYS_IOPL */

sys_iopl() is missing asmlinkage.

It would be far more conventional to use cond_syscall().  Perhaps by
adding a CONFIG_X86 area into kernel/sys_ni.c

fyi, I'm offering special deals on checkpatch.pl site licenses this month.

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[H. Peter Anvin]

On 07/14/2011 03:31 PM, Andrew Morton wrote:
> 
> sys_iopl() is missing asmlinkage.
> 
> It would be far more conventional to use cond_syscall().  Perhaps by
> adding a CONFIG_X86 area into kernel/sys_ni.c
> 
> fyi, I'm offering special deals on checkpatch.pl site licenses this month.

Again, I don't think this makes sense as a compile-time-only option.
	
	-hpa

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Thu, Jul 14, 2011 at 3:35 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/14/2011 03:31 PM, Andrew Morton wrote:
>>
>> sys_iopl() is missing asmlinkage.
>>
>> It would be far more conventional to use cond_syscall().  Perhaps by
>> adding a CONFIG_X86 area into kernel/sys_ni.c
>>
>> fyi, I'm offering special deals on checkpatch.pl site licenses this month.
>
> Again, I don't think this makes sense as a compile-time-only option.

echo "enabled" > /proc/sys/kernel/iopl_available
echo "disabled" > /proc/sys/kernel/iopl_available
echo "locked" > /proc/sys/kernel/iopl_available

?

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Alan Cox]

On Thu, 14 Jul 2011 13:34:53 -0700
Mike Waychison <mikew@google.com> wrote:

> In some build environments, it is useful to allow disabling of IO
> accesses to hardware, without having to rely on CAP_SYS_RAWIO (which is

And others include mmap and the tty driver and the PCI config space
(various devices can be manipulated via pci config space to do I/O cycles)

It strikes me that

a) you can do this with a security module
b) its rather incomplete

and as such you don't need kernel hacks to do it because everything you
want is already there.

Alan

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[H. Peter Anvin]

On 07/14/2011 03:40 PM, Mike Waychison wrote:
> On Thu, Jul 14, 2011 at 3:35 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 07/14/2011 03:31 PM, Andrew Morton wrote:
>>>
>>> sys_iopl() is missing asmlinkage.
>>>
>>> It would be far more conventional to use cond_syscall().  Perhaps by
>>> adding a CONFIG_X86 area into kernel/sys_ni.c
>>>
>>> fyi, I'm offering special deals on checkpatch.pl site licenses this month.
>>
>> Again, I don't think this makes sense as a compile-time-only option.
> 
> echo "enabled" > /proc/sys/kernel/iopl_available
> echo "disabled" > /proc/sys/kernel/iopl_available
> echo "locked" > /proc/sys/kernel/iopl_available
> 

I'm suspecting that it might be cleaner to have kernel/ioaccess and
kernel/ioaccess_lock as two booleans (0 or 1)...

	-hpa

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Thu, Jul 14, 2011 at 3:42 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Thu, 14 Jul 2011 13:34:53 -0700
> Mike Waychison <mikew@google.com> wrote:
>
>> In some build environments, it is useful to allow disabling of IO
>> accesses to hardware, without having to rely on CAP_SYS_RAWIO (which is
>
> And others include mmap and the tty driver and the PCI config space
> (various devices can be manipulated via pci config space to do I/O cycles)
>
> It strikes me that
>
> a) you can do this with a security module

I can?   How?   The whole LSM approach seems intractable to me.

> b) its rather incomplete
>
> and as such you don't need kernel hacks to do it because everything you
> want is already there.

Where?  In an out of tree security module patchset?

>
> Alan
>

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Alan Cox]

> > a) you can do this with a security module
> 
> I can?   How?   The whole LSM approach seems intractable to me.

It would certainly need some trivial tweaking (to be specific we'd need
to move from capable(x) to capable_syscall(x, syscall_code) for those
interfaces that mattered, but that would probably be a good thing anyway
from the point of view of beating the capability model into something more
flexible and would help stuff like SELinux as well I think.

We have an underlying separation of security from the other details - we
really should keep it clean that way.

Alan

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Alan Cox]

> I'm suspecting that it might be cleaner to have kernel/ioaccess and
> kernel/ioaccess_lock as two booleans (0 or 1)...

I think firstly you need to decide what you are actually trying to stop
and the scope the problem out properly. Stopping iopl and ioperm without
thinking about the bigger picture is just going to produce nerf security.

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Alan Cox]

On Thu, 14 Jul 2011 15:40:03 -0700
Mike Waychison <mikew@google.com> wrote:

> On Thu, Jul 14, 2011 at 3:35 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > On 07/14/2011 03:31 PM, Andrew Morton wrote:
> >>
> >> sys_iopl() is missing asmlinkage.
> >>
> >> It would be far more conventional to use cond_syscall().  Perhaps by
> >> adding a CONFIG_X86 area into kernel/sys_ni.c
> >>
> >> fyi, I'm offering special deals on checkpatch.pl site licenses this month.
> >
> > Again, I don't think this makes sense as a compile-time-only option.
> 
> echo "enabled" > /proc/sys/kernel/iopl_available
> echo "disabled" > /proc/sys/kernel/iopl_available
> echo "locked" > /proc/sys/kernel/iopl_available

In all these cases if I can obtain module loading permission or I use the
privileges required to operate these calls for something else then I can
replace them anyway, so "locked" is meaningless.

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Thu, Jul 14, 2011 at 4:00 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> > a) you can do this with a security module
>>
>> I can?   How?   The whole LSM approach seems intractable to me.
>
> It would certainly need some trivial tweaking (to be specific we'd need
> to move from capable(x) to capable_syscall(x, syscall_code) for those
> interfaces that mattered, but that would probably be a good thing anyway
> from the point of view of beating the capability model into something more
> flexible and would help stuff like SELinux as well I think.

The idea of building more obtuse logic on top of posix capabilities
made me puke in my mouth :(

>
> We have an underlying separation of security from the other details - we
> really should keep it clean that way.

An aspect oriented approach to security is probably fine for
environments where you want to allow somebody access to features.

In my case, I simply don't want these "features", which is why I took
the compile time approach to turning this stuff off.  I realize that
these syscalls (and the /dev/port interface) are not comprehensive (I
didn't say they were either).  I'm happy though to take suggestions
for stuff I probably should be disabling considering my goal of making
it difficult for root to compromise a system.  And yes, modules are
disabled :)

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Alan Cox]

> In my case, I simply don't want these "features", which is why I took
> the compile time approach to turning this stuff off.  I realize that
> these syscalls (and the /dev/port interface) are not comprehensive (I
> didn't say they were either).  I'm happy though to take suggestions

Indeed - but from the point of view of doing the job to a standard for
an upstream kernel there ought to be a meaningful testable definition of
what the security shift you achieve is.

> for stuff I probably should be disabling considering my goal of making
> it difficult for root to compromise a system.  And yes, modules are
> disabled :)

If you have CAP_SYS_RAWIO and some of the other interfaces you only think
it is - the kiddies toolkits already include out of the box direct module
loading hacks (in fact its fairly easy if you've got GPU PCI access to
just put the module into video memory so that only the patching needs to
be done and the module internal addresses are all fixed and can be
arranged on a suitably convenient target address)

So really there needs to be a definition of what you are trying to
achieve. My own guess is that for

	"Disallow direct access paths to hardware"

the actual answer is 'revoke RAWIO' and then give it back to very
specific selected code in very specific selected ways or possibly in some
cases where rawio is needed for stuff that shouldn't be by writing new
driver bits to provide the proper interface that we are lacking ?

So lets turn the question around a moment - what breaks if you simply
take CAP_SYS_RAWIO away from everything ?

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Thu, Jul 14, 2011 at 4:39 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> In my case, I simply don't want these "features", which is why I took
>> the compile time approach to turning this stuff off.  I realize that
>> these syscalls (and the /dev/port interface) are not comprehensive (I
>> didn't say they were either).  I'm happy though to take suggestions
>
> Indeed - but from the point of view of doing the job to a standard for
> an upstream kernel there ought to be a meaningful testable definition of
> what the security shift you achieve is.
>
>> for stuff I probably should be disabling considering my goal of making
>> it difficult for root to compromise a system.  And yes, modules are
>> disabled :)
>
> If you have CAP_SYS_RAWIO and some of the other interfaces you only think
> it is - the kiddies toolkits already include out of the box direct module
> loading hacks (in fact its fairly easy if you've got GPU PCI access to
> just put the module into video memory so that only the patching needs to
> be done and the module internal addresses are all fixed and can be
> arranged on a suitably convenient target address)
>
> So really there needs to be a definition of what you are trying to
> achieve. My own guess is that for
>
>        "Disallow direct access paths to hardware"
>
> the actual answer is 'revoke RAWIO' and then give it back to very
> specific selected code in very specific selected ways or possibly in some
> cases where rawio is needed for stuff that shouldn't be by writing new
> driver bits to provide the proper interface that we are lacking ?
>
> So lets turn the question around a moment - what breaks if you simply
> take CAP_SYS_RAWIO away from everything ?
>

Alright, I see your point.   ISTR that CAP_SYS_RAWIO was required for
accessing block devices directly, but this doesn't seem to be the
case.

I think the approach I'll try next is to try and drop it with
PR_CAPBSET_DROP from early userspace's init.

Any other vectors you would suggest to keep the kiddies away?

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Greg KH]

On Thu, Jul 14, 2011 at 03:17:24PM -0700, Mike Waychison wrote:
> On Thu, Jul 14, 2011 at 2:21 PM, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Jul 14, 2011 at 01:34:52PM -0700, Mike Waychison wrote:
> >> It would be useful to build kernels with disabled support of /dev/port
> >> for systems where we do not want to allow an administrator to access IO
> >> ports directly.
> >>
> >> To do so however, CONFIG_DEVPORT needs to be user-selectable.  Give this
> >> configuration option a name and help description.
> >>
> >> Google-Bug-Id: 3177114
> >
> > What is that field?  It's not something that I've ever seen before...
> 
> This is for our internal reference.

Internal reference to where?

> > Please don't push internal fields like this to public patches, it only
> > causes confusion...
> 
> It hasn't caused any problems so far.  It helps us track things as we
> pull code back downstream.

Ok, but what happens if all 2000+ different companies start adding their
fields to the signed-off-by: area of the kernel for their internal
tracking systems?

So please, don't do this, it makes no sense to the kernel community at
all and we really don't want to start adding more fields to this area if
we don't have to.

greg k-h

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Alan Cox]

> > So lets turn the question around a moment - what breaks if you simply
> > take CAP_SYS_RAWIO away from everything ?
> >
> 
> Alright, I see your point.   ISTR that CAP_SYS_RAWIO was required for
> accessing block devices directly, but this doesn't seem to be the
> case.

Its needed for certain kinds of command issue (raw commands) but not I
believe raw block I/O

> I think the approach I'll try next is to try and drop it with
> PR_CAPBSET_DROP from early userspace's init.
> 
> Any other vectors you would suggest to keep the kiddies away?

/dev/mem /dev/kmem (which do have options to clamp down on)
module loading you have down

GPU if you don't need any graphics. Not so much because its a specific
threat area but because its a large exposure.

For media there are exploit paths to consider where an attacker rewrites
the boot media and reboots. If you need raw disk I/O those are a spot
harder to mend.

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Theodore Tso]

On Jul 15, 2011, at 2:41 AM, Greg KH wrote:

> 
> Ok, but what happens if all 2000+ different companies start adding their
> fields to the signed-off-by: area of the kernel for their internal
> tracking systems?

That's been happening for a long time; there have been kernel patches
referencing LTC, RHEL, and SLES bugzilla entries; many of those
bug entries are not public so as not to reveal customer confidential
information.   I've never seen anyone complain about that before.

-- Ted

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Greg KH]

On Fri, Jul 15, 2011 at 09:13:27AM -0400, Theodore Tso wrote:
> 
> On Jul 15, 2011, at 2:41 AM, Greg KH wrote:
> 
> > 
> > Ok, but what happens if all 2000+ different companies start adding their
> > fields to the signed-off-by: area of the kernel for their internal
> > tracking systems?
> 
> That's been happening for a long time; there have been kernel patches
> referencing LTC, RHEL, and SLES bugzilla entries; many of those
> bug entries are not public so as not to reveal customer confidential
> information.   I've never seen anyone complain about that before.

But none of them are on the Signed-off-by: line area, right?

Also, putting it in the text of the changelog, with a url, is fine.  And
it had better be a public url, otherwise it makes no sense at all.  I've
seen many public SUSE and Ubuntu urls in changelog entries, any
non-public ones should not be in there at all.

Just like with gerrit ids in signed-off-by areas, references to internal
bugzilla entries, in the signed-off-by area, should not be there at all.

greg k-h

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Alan Cox]

> But none of them are on the Signed-off-by: line area, right?

We have a fair number of people using things like

Fixes-bug: [URL]


Its useful public info, it makes it easier to grep

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Greg KH]

On Fri, Jul 15, 2011 at 03:58:35PM +0100, Alan Cox wrote:
> > But none of them are on the Signed-off-by: line area, right?
> 
> We have a fair number of people using things like
> 
> Fixes-bug: [URL]
> 
> 
> Its useful public info, it makes it easier to grep

That's fine, but that is not what was done here.  And those URLs had
better be public as well.

greg k-h

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Mike Waychison]

On Fri, Jul 15, 2011 at 8:19 AM, Greg KH <gregkh@suse.de> wrote:
> On Fri, Jul 15, 2011 at 03:58:35PM +0100, Alan Cox wrote:
>> > But none of them are on the Signed-off-by: line area, right?
>>
>> We have a fair number of people using things like
>>
>> Fixes-bug: [URL]
>>
>>
>> Its useful public info, it makes it easier to grep
>
> That's fine, but that is not what was done here.  And those URLs had
> better be public as well.
>

Greg, this is a bit ridiculous.  If adding a bug reference number to a
patch isn't used in lieu of a good patch description, I don't see how
this hurts anybody in the public.  You're only making it more
difficult for those who actually want to contribute to the public
sources.

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Greg KH]

On Fri, Jul 15, 2011 at 09:45:20AM -0700, Mike Waychison wrote:
> On Fri, Jul 15, 2011 at 8:19 AM, Greg KH <gregkh@suse.de> wrote:
> > On Fri, Jul 15, 2011 at 03:58:35PM +0100, Alan Cox wrote:
> >> > But none of them are on the Signed-off-by: line area, right?
> >>
> >> We have a fair number of people using things like
> >>
> >> Fixes-bug: [URL]
> >>
> >>
> >> Its useful public info, it makes it easier to grep
> >
> > That's fine, but that is not what was done here.  And those URLs had
> > better be public as well.
> >
> 
> Greg, this is a bit ridiculous.  If adding a bug reference number to a
> patch isn't used in lieu of a good patch description, I don't see how
> this hurts anybody in the public.  You're only making it more
> difficult for those who actually want to contribute to the public
> sources.

What?  Come on now, do you seriously want to start seeing _every_
company put random things in the signed-off-by area depending on their
internal development workflow that has _nothing_ to do with the kernel
development community?

You do realize just how many different companies contribute every year,
right?

The information in a git commit is for the developers of the kernel, the
community, not for the individual companies that might contribute.

We need consistancy in commit logs, and by putting stuff like this in
them, in the area that is parsed by tools, that don't fit any rhyme or
reason, causes problems.  Look at the discussion that took place to just
figure out how to properly reference email threads that result in a
patch.  We worked it out, right?  But that was so we all can come to a
common goal and understanding.

Unless you feel we should come up with something like:
	Internal-reference-id: XXXXX
and use a general tag like that for all companies, please don't put
company-specific and internal references in a place where they will be
commited to the public tree.

Personally, I want to see the information in git commits to be useful
for everyone, and not reference private information, as that helps no
one except a very tiny minority of the community out, which, in my
opinion, is very selfish of them.

And yes, this means that if someone sees a reference to a private
bugzilla url, then that should be fixed either by making that bug open,
or removing that url from the git commit area.

thanks,

greg k-h

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Mike Waychison]

On Fri, Jul 15, 2011 at 10:01 AM, Greg KH <gregkh@suse.de> wrote:
> On Fri, Jul 15, 2011 at 09:45:20AM -0700, Mike Waychison wrote:
>> On Fri, Jul 15, 2011 at 8:19 AM, Greg KH <gregkh@suse.de> wrote:
>> > On Fri, Jul 15, 2011 at 03:58:35PM +0100, Alan Cox wrote:
>> >> > But none of them are on the Signed-off-by: line area, right?
>> >>
>> >> We have a fair number of people using things like
>> >>
>> >> Fixes-bug: [URL]
>> >>
>> >>
>> >> Its useful public info, it makes it easier to grep
>> >
>> > That's fine, but that is not what was done here.  And those URLs had
>> > better be public as well.
>> >
>>
>> Greg, this is a bit ridiculous.  If adding a bug reference number to a
>> patch isn't used in lieu of a good patch description, I don't see how
>> this hurts anybody in the public.  You're only making it more
>> difficult for those who actually want to contribute to the public
>> sources.
>
> What?  Come on now, do you seriously want to start seeing _every_
> company put random things in the signed-off-by area depending on their
> internal development workflow that has _nothing_ to do with the kernel
> development community?
>

Considering the footer as a machine parseable audit trail, ya, I think
it's fair to extend the auditability to those who make the
contributions as a courtesy.

FWIW, we already use a _ton_ of footers for internal tracking of
changes as they go through code review, flow between rebased trees and
get merged into release trees.  I agree with you that that sort of
information isn't relevant to the usual LKML reader, and patches
posted to LKML coming from google.com don't publicize them as they
aren't very human readable either.

> You do realize just how many different companies contribute every year,
> right?
>
> The information in a git commit is for the developers of the kernel, the
> community, not for the individual companies that might contribute.

Why are you making a distinction?  The community members often *are*
individual companies.

>
> We need consistancy in commit logs, and by putting stuff like this in
> them, in the area that is parsed by tools, that don't fit any rhyme or
> reason, causes problems.  Look at the discussion that took place to just
> figure out how to properly reference email threads that result in a
> patch.  We worked it out, right?  But that was so we all can come to a
> common goal and understanding.

Did we?  I don't see it documented anywhere :(

>
> Unless you feel we should come up with something like:
>        Internal-reference-id: XXXXX
> and use a general tag like that for all companies, please don't put
> company-specific and internal references in a place where they will be
> commited to the public tree.

I'd be fine with something like "Internal-reference-id".

Really, I don't see what the problem with the format of the entry is,
I mean, it's prefixed with "Google" specifically so that doesn't clash
with other future tags.  If you feel that this needs to be into a
common tag that others can use, let's have that discussion.  I
honestly don't see what the big deal is though.

>
> Personally, I want to see the information in git commits to be useful
> for everyone, and not reference private information, as that helps no
> one except a very tiny minority of the community out, which, in my
> opinion, is very selfish of them.
>
> And yes, this means that if someone sees a reference to a private
> bugzilla url, then that should be fixed either by making that bug open,
> or removing that url from the git commit area.

The next time I'm on the stand trying to explain to a jury in federal
court why kernel developer X made change Y to Linux, I don't want to
have to explain that we can't find that information because keeping
the audit trail didn't align with your personal preferences.

Mike Waychison

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[H. Peter Anvin]

On 07/15/2011 10:51 AM, Mike Waychison wrote:
>>
>> Unless you feel we should come up with something like:
>>        Internal-reference-id: XXXXX
>> and use a general tag like that for all companies, please don't put
>> company-specific and internal references in a place where they will be
>> commited to the public tree.
> 
> I'd be fine with something like "Internal-reference-id".
> 

I would go even further, and do something like:

Patch-ID: <id@sender> ...

... if it has internal structure to the poster that would be up to them.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Thu, Jul 14, 2011 at 5:48 PM, Mike Waychison <mikew@google.com> wrote:
> On Thu, Jul 14, 2011 at 4:39 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

>>> for stuff I probably should be disabling considering my goal of making
>>> it difficult for root to compromise a system.  And yes, modules are
>>> disabled :)
>>
>> If you have CAP_SYS_RAWIO and some of the other interfaces you only think
>> it is - the kiddies toolkits already include out of the box direct module
>> loading hacks (in fact its fairly easy if you've got GPU PCI access to
>> just put the module into video memory so that only the patching needs to
>> be done and the module internal addresses are all fixed and can be
>> arranged on a suitably convenient target address)
>>
>> So really there needs to be a definition of what you are trying to
>> achieve. My own guess is that for
>>
>>        "Disallow direct access paths to hardware"
>>
>> the actual answer is 'revoke RAWIO' and then give it back to very
>> specific selected code in very specific selected ways or possibly in some
>> cases where rawio is needed for stuff that shouldn't be by writing new
>> driver bits to provide the proper interface that we are lacking ?
>>
>> So lets turn the question around a moment - what breaks if you simply
>> take CAP_SYS_RAWIO away from everything ?
>>
>
> Alright, I see your point.   ISTR that CAP_SYS_RAWIO was required for
> accessing block devices directly, but this doesn't seem to be the
> case.
>
> I think the approach I'll try next is to try and drop it with
> PR_CAPBSET_DROP from early userspace's init.
>

For my use-case, I'd like to have a system boot with a non-default
bounding set of posix capabilities.  I'd like the system to *never* be
able to use these capabilities, so I'd like to drop them early on when
userland starts up.  Given this requirement (and the fact that
capability manipulation is process-local), I'd like for my init setup
to drop certain bits from the bounding set early on during bootup.

I'm thinking of adding the following linux command line flag:

capbset_drop=<comma separated list of capabilities>

example:

capbset_drop=CAP_SYS_RAWIO
capbset_drop=CAP_SYS_RAWIO,CAP_NET_RAW

I'm thinking that this option would drop the listed capabilities from
the bounding set, as well as init's permitted, effective and inherited
masks.

I'd probably want to eventually also provide a way to set the
securebits (they seem to operate in the same way?), though for now I'd
rather tackle the capability masks directly.

So the question is, should this go in the kernel proper such that it
manipulates the init_cred structure, or should this be plumbed down in
kinit (in klibc, which we use for bootup)?

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[H. Peter Anvin]

On 07/15/2011 11:13 AM, Mike Waychison wrote:
> 
> So the question is, should this go in the kernel proper such that it
> manipulates the init_cred structure, or should this be plumbed down in
> kinit (in klibc, which we use for bootup)?
> 

It certainly would be trivial to do in kinit.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Andrew Morton]

On Fri, 15 Jul 2011 15:58:35 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > But none of them are on the Signed-off-by: line area, right?
> 
> We have a fair number of people using things like
> 
> Fixes-bug: [URL]

I use

	Addresses http://bugzilla....

> Its useful public info, it makes it easier to grep

I don't think this issue is worth the knicker-knotting.  If someone
really wants to see the original report they can send an email to the
patch author asking for it, shrug.  If the organization-specific tag
is omitted, they'd never think to do that.

What concerns me more (and which happens a lot more often) is people
forgetting to include any reference to the original bug report.  And
people forgetting Reported-by:.

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Ted Ts'o]

On Fri, Jul 15, 2011 at 04:51:32PM +0200, Greg KH wrote:
> 
> But none of them are on the Signed-off-by: line area, right?

Is it really the case that your concern is that there be a whitespace
between the bug tracker reference and the signed-off-by area?   So

     Company-bug-id: ....

     Signed-off-by: ....

is preferable to

     Company-bug-id:
     Signed-off-by: ...

If that's your only concern, ok, whatever.  I don't personally care a
whole lot either way.

It's not like there was ever any kind of formal standards committee
that decided that adding "Acked-by" and "Tested-by" and "CC" were ok
in the signed-off-by block, and other things weren't, after all.

       		     	    	      	     	      - Ted

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Andrew G. Morgan]

I'd put it in kinit too.

I think you may have to think about the call_usermodehelper code, and
you might want to look at dropping CAP_SYS_MODULE too.

Cheers

Andrew

On Fri, Jul 15, 2011 at 11:14 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/15/2011 11:13 AM, Mike Waychison wrote:
>>
>> So the question is, should this go in the kernel proper such that it
>> manipulates the init_cred structure, or should this be plumbed down in
>> kinit (in klibc, which we use for bootup)?
>>
>
> It certainly would be trivial to do in kinit.
>
>        -hpa
>
> --
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
>
>

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Mike Waychison]

On Fri, Jul 15, 2011 at 3:30 PM, Andrew G. Morgan <agm@google.com> wrote:
> I'd put it in kinit too.
>
> I think you may have to think about the call_usermodehelper code, and
> you might want to look at dropping CAP_SYS_MODULE too.

Looks like usermodehelpers are configurable for both the inheritable
set and the bounding set via /proc/sys/kernel/usermodehelper/bset and
/proc/sys/kernel/usermodehelper/inheritable thanks to Eric Paris
(17f60a7da, available in 3.0-rc1).

>
> Cheers
>
> Andrew
>
> On Fri, Jul 15, 2011 at 11:14 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 07/15/2011 11:13 AM, Mike Waychison wrote:
>>>
>>> So the question is, should this go in the kernel proper such that it
>>> manipulates the init_cred structure, or should this be plumbed down in
>>> kinit (in klibc, which we use for bootup)?
>>>
>>
>> It certainly would be trivial to do in kinit.
>>
>>        -hpa
>>
>> --
>> H. Peter Anvin, Intel Open Source Technology Center
>> I work for Intel.  I don't speak on their behalf.
>>
>>
>

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Greg KH]

On Fri, Jul 15, 2011 at 02:55:19PM -0400, Ted Ts'o wrote:
> On Fri, Jul 15, 2011 at 04:51:32PM +0200, Greg KH wrote:
> > 
> > But none of them are on the Signed-off-by: line area, right?
> 
> Is it really the case that your concern is that there be a whitespace
> between the bug tracker reference and the signed-off-by area?   So
> 
>      Company-bug-id: ....
> 
>      Signed-off-by: ....
> 
> is preferable to
> 
>      Company-bug-id:
>      Signed-off-by: ...
> 
> If that's your only concern, ok, whatever.  I don't personally care a
> whole lot either way.
> 
> It's not like there was ever any kind of formal standards committee
> that decided that adding "Acked-by" and "Tested-by" and "CC" were ok
> in the signed-off-by block, and other things weren't, after all.

But those were agreed apon, and standardized on, and look, documented,
as to how to use them.

If you want to propose, and document, using "Company-bug-id", that's
great, and is valid, but don't try to slip things in like this, with a
company specific name, into the standardized tag area and claim it is
acceptable after the fact.

Again, I think people here are somehow forgetting the 300+ different
companies that contribute to the kernel...

greg k-h

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Ted Ts'o]

On Sat, Jul 16, 2011 at 09:56:00AM +0200, Greg KH wrote:
> 
> If you want to propose, and document, using "Company-bug-id", that's
> great, and is valid, but don't try to slip things in like this, with a
> company specific name, into the standardized tag area and claim it is
> acceptable after the fact.
> 
> Again, I think people here are somehow forgetting the 300+ different
> companies that contribute to the kernel...

I've been doing this for several years, and there have been times when
I've put in Bugzilla references to both RedHat *and* SuSE bugs that
were closed due to customer confidential information.  I had access
because of NDA's signed between IBM and the enterprise distro's, and
the release engineers at both distro's seemed to be happy that I did
this.  In some cases I didn't have access to the bugzilla entry in
question, but I added the reference out of courtesy to the help desk
folks at the distro in question so they could more easily track the
fix as it travelled upstream to Linus and to various other derivitive
kernels, including the stable kernel series.

This is not new.  I've tended not to put it in the signed-off-block,
so when I've done it it's just been part of the commit description.

If it eases friction to some subset of the people who are using the
kernel, especially to people who helped me debug a problem in ext3,
ext4, e2fsprogs, etc., it seems like it's just common courtesy to
include a single line so the bugfix can be more easily tracked.  If
they helped me, I should help them.

I certainly don't plan to change this for anything for which I'm the
maintainer, and I would encourage other maintainers to do the same.

    	  		  		       - Ted

Re: [PATCH 1/2] Kconfig: Allow disabling of CONFIG_DEVPORT

[Christoph Hellwig]

FYI, back when SGI was maintained in SGI ptools, which required a bug
to check in we always had the SGI Bugworks reference, and the ptools
commit description in the commits, with none of that beeing publically
available, and no one comaplined.

Example:

commit f1ccd2955157e1aff992f6aaaba0944209076220
Author: Lachlan McIlroy <lachlan@sgi.com>
Date:   Fri Sep 26 12:16:46 2008 +1000

    [XFS] Fix extent list corruption in xfs_iext_irec_compact_full().

    If we don't move all the records from the next buffer into the current
    buffer then we need to update the er_extoff field of the next buffer as we
    shift the remaining records to the start of the buffer.

    SGI-PV: 987159

    SGI-Modid: xfs-linux-melb:xfs-kern:32165a
				        
    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>
    Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Eric Paris]

On Sat, Jul 16, 2011 at 12:42 AM, Mike Waychison <mikew@google.com> wrote:
> On Fri, Jul 15, 2011 at 3:30 PM, Andrew G. Morgan <agm@google.com> wrote:
>> I'd put it in kinit too.
>>
>> I think you may have to think about the call_usermodehelper code, and
>> you might want to look at dropping CAP_SYS_MODULE too.
>
> Looks like usermodehelpers are configurable for both the inheritable
> set and the bounding set via /proc/sys/kernel/usermodehelper/bset and
> /proc/sys/kernel/usermodehelper/inheritable thanks to Eric Paris
> (17f60a7da, available in 3.0-rc1).

If you look in Fedora and RHEL you'll see that we actually already
provide a dracut module (dracut-caps) which can be used to create an
initrd which contains all of the modules you need to load, it loads
them, and then will drop all of the caps that you want to drop.   Good
to see we already solved this problem once!!  (although it requires
that your kernel and initrd not be in a place that your root user can
modify it, easy to do in the virt space, no so easy in the real
hardware world)

-Eric

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[H. Peter Anvin]

On 07/17/2011 04:19 PM, Eric Paris wrote:
> On Sat, Jul 16, 2011 at 12:42 AM, Mike Waychison <mikew@google.com> wrote:
>> On Fri, Jul 15, 2011 at 3:30 PM, Andrew G. Morgan <agm@google.com> wrote:
>>> I'd put it in kinit too.
>>>
>>> I think you may have to think about the call_usermodehelper code, and
>>> you might want to look at dropping CAP_SYS_MODULE too.
>>
>> Looks like usermodehelpers are configurable for both the inheritable
>> set and the bounding set via /proc/sys/kernel/usermodehelper/bset and
>> /proc/sys/kernel/usermodehelper/inheritable thanks to Eric Paris
>> (17f60a7da, available in 3.0-rc1).
> 
> If you look in Fedora and RHEL you'll see that we actually already
> provide a dracut module (dracut-caps) which can be used to create an
> initrd which contains all of the modules you need to load, it loads
> them, and then will drop all of the caps that you want to drop.   Good
> to see we already solved this problem once!!  (although it requires
> that your kernel and initrd not be in a place that your root user can
> modify it, easy to do in the virt space, no so easy in the real
> hardware world)
> 

Have you considered separating out the kernel-specific portions into a
separate initramfs file (preferrably one which could be built from a
kernel build tree).  The whole dependency of kernels with initramfs is a
huge pain for kernel development and debugging...

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Jiri Kosina]

On Thu, 14 Jul 2011, H. Peter Anvin wrote:

> >> Wouldn't it be more useful for this to be a sysctl?  In particular, like
> >> many similar things it probably should be a lockable sysctl (three
> >> states: enabled, disabled, and locked-disabled).
> >>
> >> Making it a compile-time option I'm very skeptical to.
> > 
> > Are there existing examples of this already in the tree?
> 
> I think so, but I don't know off the top of my head.

/proc/sys/kernel/modules_disabled is a sort-of that kind of thing (it 
doesn't have three states, but only two -- enabled and locked-disabled).

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] x86: Allow disabling of sys_iopl, sys_ioperm

[Ingo Molnar]

* Mike Waychison <mikew@google.com> wrote:

> On Thu, Jul 14, 2011 at 3:35 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > On 07/14/2011 03:31 PM, Andrew Morton wrote:
> >>
> >> sys_iopl() is missing asmlinkage.
> >>
> >> It would be far more conventional to use cond_syscall().  Perhaps by
> >> adding a CONFIG_X86 area into kernel/sys_ni.c
> >>
> >> fyi, I'm offering special deals on checkpatch.pl site licenses this month.
> >
> > Again, I don't think this makes sense as a compile-time-only option.
> 
> echo "enabled" > /proc/sys/kernel/iopl_available
> echo "disabled" > /proc/sys/kernel/iopl_available
> echo "locked" > /proc/sys/kernel/iopl_available

1, 0 and -1 ought to be a rather straightforward mapping to 'on', 
'off' and 'off forever'.

Thanks,

	Ingo