[PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

Hi,

likely a merge error by patch ... which makes setuid_dumpable
appear in /proc/sys/fs/ rather than kernel/. I assume it belongs
to kernel as the naming suggests.

Patch is against 2.6.16-rc5-git9.

From: Kurt Garloff <garloff@suse.de>
Subject: suid-dumpable ended up in wrong sysctl dir

Diffing in sysctl.c is tricky, using more context is recommended.
suid_dumpable ended up in fs/ instead of kernel/ and the reason
is likely a patch with too little context.

Signed-off-by: Kurt Garloff <garloff@suse.de>

Index: linux-2.6.15/kernel/sysctl.c
===================================================================
--- linux-2.6.15.orig/kernel/sysctl.c
+++ linux-2.6.15/kernel/sysctl.c
@@ -647,12 +647,20 @@ static ctl_table kern_table[] = {
 		.data		= &randomize_va_space,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
 #endif
+	{
+		.ctl_name	= KERN_SETUID_DUMPABLE,
+		.procname	= "suid_dumpable",
+		.data		= &suid_dumpable,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
 #if defined(CONFIG_S390) && defined(CONFIG_SMP)
 	{
 		.ctl_name	= KERN_SPIN_RETRY,
 		.procname	= "spin_retry",
 		.data		= &spin_retry,
 		.maxlen		= sizeof (int),
@@ -1032,20 +1040,12 @@ static ctl_table fs_table[] = {
 		.procname	= "inotify",
 		.mode		= 0555,
 		.child		= inotify_table,
 	},
 #endif	
 #endif
-	{
-		.ctl_name	= KERN_SETUID_DUMPABLE,
-		.procname	= "suid_dumpable",
-		.data		= &suid_dumpable,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= &proc_dointvec,
-	},
 	{ .ctl_name = 0 }
 };
 
 static ctl_table debug_table[] = {
 	{ .ctl_name = 0 }
 };
-- 
Kurt Garloff, Head Architect Linux, Novell Inc.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Andrew Morton]

Kurt Garloff <garloff@suse.de> wrote:
>
> Diffing in sysctl.c is tricky, using more context is recommended.
> suid_dumpable ended up in fs/ instead of kernel/ and the reason
> is likely a patch with too little context.

It's been in kernel/ since 2.6.13.  What will break if we move it?

This is security-related.  If we move it we risk unsecuring people's
machines...

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Arjan van de Ven]

On Fri, 2006-03-10 at 14:56 -0800, Andrew Morton wrote:
> Kurt Garloff <garloff@suse.de> wrote:
> >
> > Diffing in sysctl.c is tricky, using more context is recommended.
> > suid_dumpable ended up in fs/ instead of kernel/ and the reason
> > is likely a patch with too little context.
> 
> It's been in kernel/ since 2.6.13.  What will break if we move it?
> 
> This is security-related.  If we move it we risk unsecuring people's
> machines...

only a very little bit since the default value is "secure", the option
is to make it "insecure"...
but yeah by this time we should just bite the bullet and rename the
variable rather than move it about

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Andrew Morton]

Arjan van de Ven <arjan@infradead.org> wrote:
>
> On Fri, 2006-03-10 at 14:56 -0800, Andrew Morton wrote:
> > Kurt Garloff <garloff@suse.de> wrote:
> > >
> > > Diffing in sysctl.c is tricky, using more context is recommended.
> > > suid_dumpable ended up in fs/ instead of kernel/ and the reason
> > > is likely a patch with too little context.
> > 
> > It's been in kernel/ since 2.6.13.  What will break if we move it?
> > 
> > This is security-related.  If we move it we risk unsecuring people's
> > machines...
> 
> only a very little bit since the default value is "secure", the option
> is to make it "insecure"...

OK, that's a good point.

> but yeah by this time we should just bite the bullet and rename the
> variable rather than move it about

That wouldn't help - we'll still break existing scripts.

crap.  I tend to think we leave it where it is - it's only a cosmetic
irritation, isn't it?

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Arjan van de Ven]

> 
> > but yeah by this time we should just bite the bullet and rename the
> > variable rather than move it about
> 
> That wouldn't help - we'll still break existing scripts.

why? (I mean the KERN_ to FS_ rename in the kernel, that's not exported
to userland anywhere)

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Andrew Morton]

Arjan van de Ven <arjan@infradead.org> wrote:
>
> 
> > 
> > > but yeah by this time we should just bite the bullet and rename the
> > > variable rather than move it about
> > 
> > That wouldn't help - we'll still break existing scripts.
> 
> why? (I mean the KERN_ to FS_ rename in the kernel, that's not exported
> to userland anywhere)

oic.  What are you expecting here?  Reading skills?

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Arjan van de Ven]

On Fri, 2006-03-10 at 23:51 -0800, Andrew Morton wrote:
> Arjan van de Ven <arjan@infradead.org> wrote:
> >
> > 
> > > 
> > > > but yeah by this time we should just bite the bullet and rename the
> > > > variable rather than move it about
> > > 
> > > That wouldn't help - we'll still break existing scripts.
> > 
> > why? (I mean the KERN_ to FS_ rename in the kernel, that's not exported
> > to userland anywhere)
> 
> oic.  What are you expecting here?  Reading skills?

lesson to self: only speak in diff -u form before morning coffee


the setuid-dumpable sysctl got misplaces due to a patch glitch. But it's
part of the ABI now for some time so we need to keep it. This patch at
least renames the variable inside the kernel to match the new place, and
puts a comment in place to indicate that it's a known glitch

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

diff -purN linux-2.6.16-rc5/include/linux/sysctl.h linux-2.6.16-rc5-setuid/include/linux/sysctl.h
--- linux-2.6.16-rc5/include/linux/sysctl.h	2006-02-27 09:13:31.000000000 +0100
+++ linux-2.6.16-rc5-setuid/include/linux/sysctl.h	2006-03-11 09:02:13.000000000 +0100
@@ -144,7 +144,6 @@ enum
 	KERN_UNKNOWN_NMI_PANIC=66, /* int: unknown nmi panic flag */
 	KERN_BOOTLOADER_TYPE=67, /* int: boot loader type */
 	KERN_RANDOMIZE=68, /* int: randomize virtual address space */
-	KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core */
 	KERN_SPIN_RETRY=70,	/* int: number of spinlock retries */
 	KERN_ACPI_VIDEO_FLAGS=71, /* int: flags for setting up video after ACPI sleep */
 };
@@ -759,6 +758,9 @@ enum
 	FS_AIO_NR=18,	/* current system-wide number of aio requests */
 	FS_AIO_MAX_NR=19,	/* system-wide maximum number of aio requests */
 	FS_INOTIFY=20,	/* inotify submenu */
+			/* Note: the following got misplaced but this mistake is 
+			   now part of the ABI */
+	FS_SETUID_DUMPABLE=21, /* int: behaviour of dumps for setuid core */
 };
 
 /* /proc/sys/fs/quota/ */
diff -purN linux-2.6.16-rc5/kernel/sysctl.c linux-2.6.16-rc5-setuid/kernel/sysctl.c
--- linux-2.6.16-rc5/kernel/sysctl.c	2006-02-27 09:13:31.000000000 +0100
+++ linux-2.6.16-rc5-setuid/kernel/sysctl.c	2006-03-11 09:01:19.000000000 +0100
@@ -1022,7 +1022,7 @@ static ctl_table fs_table[] = {
 #endif	
 #endif
 	{
-		.ctl_name	= KERN_SETUID_DUMPABLE,
+		.ctl_name	= FS_SETUID_DUMPABLE,
 		.procname	= "suid_dumpable",
 		.data		= &suid_dumpable,
 		.maxlen		= sizeof(int),

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Jan Engelhardt]

>the setuid-dumpable sysctl got misplaces due to a patch glitch. But it's
>part of the ABI now for some time so we need to keep it. This patch at
>least renames the variable inside the kernel to match the new place, and
>puts a comment in place to indicate that it's a known glitch
>
"To be corrected to the right thing for 2.7".

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Eric W. Biederman]

Arjan van de Ven <arjan@infradead.org> writes:

>
> diff -purN linux-2.6.16-rc5/include/linux/sysctl.h
> linux-2.6.16-rc5-setuid/include/linux/sysctl.h
> --- linux-2.6.16-rc5/include/linux/sysctl.h 2006-02-27 09:13:31.000000000 +0100
> +++ linux-2.6.16-rc5-setuid/include/linux/sysctl.h 2006-03-11 09:02:13.000000000
> +0100
> @@ -144,7 +144,6 @@ enum
>  	KERN_UNKNOWN_NMI_PANIC=66, /* int: unknown nmi panic flag */
>  	KERN_BOOTLOADER_TYPE=67, /* int: boot loader type */
>  	KERN_RANDOMIZE=68, /* int: randomize virtual address space */
> -	KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core */
>  	KERN_SPIN_RETRY=70,	/* int: number of spinlock retries */
>  	KERN_ACPI_VIDEO_FLAGS=71, /* int: flags for setting up video after ACPI
> sleep */
>  };
> @@ -759,6 +758,9 @@ enum
>  	FS_AIO_NR=18,	/* current system-wide number of aio requests */
>  	FS_AIO_MAX_NR=19, /* system-wide maximum number of aio requests */
>  	FS_INOTIFY=20,	/* inotify submenu */
> + /* Note: the following got misplaced but this mistake is
> +			   now part of the ABI */
> +	FS_SETUID_DUMPABLE=21, /* int: behaviour of dumps for setuid core */
This must be number 69 here.  Or else we break the sys_sysctl ABI.
>  };
>  

Eric

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

Hi Arjan, Eric,

On Sun, Mar 12, 2006 at 03:07:47PM -0700, Eric W. Biederman wrote:
> Arjan van de Ven <arjan@infradead.org> writes:
> 
> > diff -purN linux-2.6.16-rc5/include/linux/sysctl.h
> > linux-2.6.16-rc5-setuid/include/linux/sysctl.h
> > --- linux-2.6.16-rc5/include/linux/sysctl.h 2006-02-27 09:13:31.000000000 +0100
> > +++ linux-2.6.16-rc5-setuid/include/linux/sysctl.h 2006-03-11 09:02:13.000000000
> > +0100
> > @@ -144,7 +144,6 @@ enum
> >  	KERN_UNKNOWN_NMI_PANIC=66, /* int: unknown nmi panic flag */
> >  	KERN_BOOTLOADER_TYPE=67, /* int: boot loader type */
> >  	KERN_RANDOMIZE=68, /* int: randomize virtual address space */
> > -	KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core */
> >  	KERN_SPIN_RETRY=70,	/* int: number of spinlock retries */
> >  	KERN_ACPI_VIDEO_FLAGS=71, /* int: flags for setting up video after ACPI
> > sleep */
> >  };
> > @@ -759,6 +758,9 @@ enum
> >  	FS_AIO_NR=18,	/* current system-wide number of aio requests */
> >  	FS_AIO_MAX_NR=19, /* system-wide maximum number of aio requests */
> >  	FS_INOTIFY=20,	/* inotify submenu */
> > + /* Note: the following got misplaced but this mistake is
> > +			   now part of the ABI */
> > +	FS_SETUID_DUMPABLE=21, /* int: behaviour of dumps for setuid core */

OK, this is the other possibility. It'll leave a hole in the KERN_
sysctl numbering, but that's not a problem AFAICT.
I would however at least annotate with FIXME and make a note that it's
gonna change in 2.7.

It's hard to tell how many people depend on suid_dumpable being at the
wrong location. The fact nobody noticed it being placed wrongly would
make me suspect it's not widely used yet (it only got introduced
2005-06-23 by Alan Cox). So I would prefer to move it to the correct
location now rather than in 2.7. By the time 2.7 comes, we may have so
many users, we may not want to correct this any more at all.

> This must be number 69 here.  Or else we break the sys_sysctl ABI.

I don't think we break anything we want to maintain. 
Kernel-internal defines? 
OK to change when going from 2.6.15 to 2.6.16 IMVHO. No module
compiled for 2.6.15 will work with 2.6.16 anyway.

Best,
-- 
Kurt Garloff, Head Architect Linux, Novell Inc.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Eric W. Biederman]

Kurt Garloff <garloff@suse.de> writes:

> Hi Arjan, Eric,
>
> On Sun, Mar 12, 2006 at 03:07:47PM -0700, Eric W. Biederman wrote:
>> Arjan van de Ven <arjan@infradead.org> writes:
>> 
>> > diff -purN linux-2.6.16-rc5/include/linux/sysctl.h
>> > linux-2.6.16-rc5-setuid/include/linux/sysctl.h
>> > --- linux-2.6.16-rc5/include/linux/sysctl.h 2006-02-27 09:13:31.000000000
> +0100
>> > +++ linux-2.6.16-rc5-setuid/include/linux/sysctl.h 2006-03-11
> 09:02:13.000000000
>> > +0100
>> > @@ -144,7 +144,6 @@ enum
>> >  	KERN_UNKNOWN_NMI_PANIC=66, /* int: unknown nmi panic flag */
>> >  	KERN_BOOTLOADER_TYPE=67, /* int: boot loader type */
>> >  	KERN_RANDOMIZE=68, /* int: randomize virtual address space */
>> > -	KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core */
>> >  	KERN_SPIN_RETRY=70,	/* int: number of spinlock retries */
>> >  	KERN_ACPI_VIDEO_FLAGS=71, /* int: flags for setting up video after ACPI
>> > sleep */
>> >  };
>> > @@ -759,6 +758,9 @@ enum
>> >  	FS_AIO_NR=18,	/* current system-wide number of aio requests */
>> >  	FS_AIO_MAX_NR=19, /* system-wide maximum number of aio requests */
>> >  	FS_INOTIFY=20,	/* inotify submenu */
>> > + /* Note: the following got misplaced but this mistake is
>> > +			   now part of the ABI */
>> > +	FS_SETUID_DUMPABLE=21, /* int: behaviour of dumps for setuid core */
>
> OK, this is the other possibility. It'll leave a hole in the KERN_
> sysctl numbering, but that's not a problem AFAICT.
> I would however at least annotate with FIXME and make a note that it's
> gonna change in 2.7.

Making the variable show up in two places is reasonable.
Removing it from it's current location is not, as there are currently
no technical reasons to make the change.

Unless something dramatic shows up 2.7 isn't going to happen.
If you want something like that to happen use:
Documentation/feature-removal-schedule.txt


> It's hard to tell how many people depend on suid_dumpable being at the
> wrong location. The fact nobody noticed it being placed wrongly would
> make me suspect it's not widely used yet (it only got introduced
> 2005-06-23 by Alan Cox). So I would prefer to move it to the correct
> location now rather than in 2.7. By the time 2.7 comes, we may have so
> many users, we may not want to correct this any more at all.

Don't move it just add it to the correct location.  But also leave
it at it's old location.  Then put something in
Documentation/feature-removal-schedule.txt that says in six months or
so it will be gone, from the bad location.

>> This must be number 69 here.  Or else we break the sys_sysctl ABI.
>
> I don't think we break anything we want to maintain. 
> Kernel-internal defines? 

That 69 is not kernel internal it is exported to user space,
which is why changing that is wrong.

> OK to change when going from 2.6.15 to 2.6.16 IMVHO. No module
> compiled for 2.6.15 will work with 2.6.16 anyway.

No because it isn't kernel internal.  Which is why I mentioned
sys_sysctl.  It uses that 69 as part of the path to that variable.

Eric

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Arjan van de Ven]

> This must be number 69 here.  Or else we break the sys_sysctl ABI.

numeric sysctl abi is since 2.6.0 no longer an ABI though; anything
after that.. not an ABI :)

Re: [PATCH] KERN_SETUID_DUMPABLE in /proc/sys/fs/

[Eric W. Biederman]

Arjan van de Ven <arjan@infradead.org> writes:

>> This must be number 69 here.  Or else we break the sys_sysctl ABI.
>
> numeric sysctl abi is since 2.6.0 no longer an ABI though; anything
> after that.. not an ABI :)

The system call still exists, in the system call table and
is still implemented.  The system call still takes a binary path.  I
don't see any big fat deprecated warnings in Documentation.  So if the
status has changed it has not been well communicated.

Looking a little closer I can find this note in sysctl.h:

 ****************************************************************
 **
 **  The values in this file are exported to user space via 
 **  the sysctl() binary interface.  However this interface
 **  is unstable and deprecated and will be removed in the future. 
 **  For a stable interface use /proc/sys.
 **
 ****************************************************************
Which used to be:

 ****************************************************************
 ****************************************************************
 **
 **  WARNING:  
 **  The values in this file are exported to user space via 
 **  the sysctl() binary interface.  Do *NOT* change the 
 **  numbering of any existing values here, and do not change
 **  any numbers within any one set of values.  If you have
 **  to redefine an existing interface, use a new number for it.
 **  The kernel will then return ENOTDIR to any application using
 **  the old binary interface.
 **
 **  --sct
 **
 ****************************************************************

However except for new values and old values that are now reserved
because that variable is no longer supported I do not see any
changes in sysctl values from 2.4 to 2.6.

Looking in the git history of 2.6.0 at sysctl.h I can only
see one clear instance of a value being reused for a
different purpose, and that was somewhere under SCTP.

If we are going to kill the binary ABI I'm fine with that but
it should be in Documentation/feature-removal-schedule.txt
The system call should generate a rate-limited warning in
the logs, or be removed entirely from the syscall table.

None of that was done.  Given the current state of things I am
strongly tempted to just to revert the comment, at the
top of sysctl.h, as that would be easier than going through
the proper steps to remove the system call.

However on the practical side it looks like /sbin/sysctl
only uses the /proc interface.  So it looks like the number
of users of the binary interface are probably quite slim.
So now would not be a bad time to deprecate the binary system call
entirely, and in six months or whatever submit the patch to
remove it.

But until we do that we should maintain the ABI, since we
have been.  It's not like setting enum to number 69 is an onerous
task.

Eric