procfs permissions on 2.6.x

Hi Andrew!

stumbled over the following detail ...

usually when somebody tries to modify an inode, notify_change() calls inode_change_ok() to verify the user's permissions ... now it seems that somewhere around 2.5.41, a patch similar to this one was included into the mainline, and remained almost unmodified ...

http://www.uwsg.iu.edu/hypermail/linux/kernel/0210.1/1002.html

this probably unintentionally circumvents the inode_change_ok() check, so that now any user can modify inodes of the procfs.

example:

  $ chmod a-rwx /proc/cmdline

the following patch hopefully fixes this, so please consider for inclusion ...

TIA,
Herbert

diff -NurpP --minimal linux-2.6.7/fs/proc/generic.c linux-2.6.7-fix/fs/proc/generic.c
@@ -241,8 +241,20 @@ static int proc_notify_change(struct den
 	return error;
 }
 
+static int proc_setattr(struct dentry *dentry, struct iattr *iattr)
+{
+        struct inode *inode = dentry->d_inode;
+        int error;
+
+        error = inode_change_ok(inode, iattr);
+        if (error)
+                return error;
+	error = proc_notify_change(dentry, iattr);
+	return error;
+}
+
 static struct inode_operations proc_file_inode_operations = {
-	.setattr	= proc_notify_change,
+	.setattr	= proc_setattr,
 };
 
 /*

@@ -472,7 +484,7 @@ static struct file_operations proc_dir_o
  */
 static struct inode_operations proc_dir_inode_operations = {
 	.lookup		= proc_lookup,
-	.setattr	= proc_notify_change,
+	.setattr	= proc_setattr,
 };
 
 static int proc_register(struct proc_dir_entry * dir, struct proc_dir_entry * dp)

Actually the patch you reference above looks extremly bogus and should just be reverted instead.

Why is it "extremely bogus"? I assume Olaf had a reason for wanting chmod on procfs files?

Because it turns the question what permissions a procfs file has into a lottery game. He only changes the incore inode owner and as soon as the inode is reclaimed the old ones return.

procfs inodes aren't reclaimable.

chrisw fixed this bug a couple of days ago.

WTF do you mean "not reclaimable"? They do get freed under memory pressure and recreated on later lookups.

Got confused.

Some do. On my test box 1000-odd /proc inodes get allocated and fully freed on each ls -R /proc'. 65 /proc inodes are freed during ls -lR /proc/net'. So maybe it isn't working completely.

But proc_notify_change() copies the inode's uid, gid and mode into the proc_dir_entry, so they get correctly initialised when the inode is reinstantiated, so afaict we have no bug here.

Why on the earth do we ever want to allow chown/chmod on procfs in the first place?

Al, who'd missed that stuff back in 2.5.42, but would love to hear explanation anyway.

We always have done, even current 2.4 permits it. But the changes go away when the /proc file is closed.

That change made the chown/chgrp/chmod changes persist after the file is closed, which seems sensible.

The alternative would be to disallow these changes altogether. That might break something, but it seems doubtful.

As for why anyone would _want_ to change these things: no idea.

[...]

Because I don't want the whole world reading my conntrack state. And I'm sure other people have other opinions there and on other things, so that's why we should be able to change it.

--Andy

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[email protected] wrote:

| On Sun, Jul 04, 2004 at 02:55:42PM -0700, Andrew Morton wrote:
|
|>Some do.  On my test box 1000-odd /proc inodes get allocated and fully
|>freed on each `ls -R /proc'.  65 /proc inodes are freed during `ls -lR
|>/proc/net'.  So maybe it isn't working completely.
|>
|>But proc_notify_change() copies the inode's uid, gid and mode into the
|>proc_dir_entry, so they get correctly initialised when the inode is
|>reinstantiated, so afaict we have no bug here.
|
|
| Why on the earth do we ever want to allow chown/chmod on procfs in the

first
| place?

Well perhaps I am on the wrong track but eg /proc/bus/usb/002/005 is my digital camera and unless its either world rw or owned by me (user) I can't get any pictures unless I make myself root.

So yes, I would want to have chown/chmod in procfs ...

- --
Clemens Schwaighofer - IT Engineer & System Administration
==========================================================
TEQUILA\Japan, 6-17-2 Ginza Chuo-ku, Tokyo 104-8167, JAPAN
Tel: +81-(0)3-3545-7703 Fax: +81-(0)3-3545-7343
http://www.tequila.co.jp
==========================================================
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA6LPajBz/yQjBxz8RApbcAKCfmBwm92UmiAqOZvEtZq6M215XKACg4Tbl oF+mx5LOEd9QMrrVomg+lOY=
=qvWj
-----END PGP SIGNATURE-----

... except that /proc/bus/usb/* is not in procfs. BTW, it makes more sense to change GID of the file in question and make it group-writable, adding the users who are allowed to mess with it into the group.

Hi Clemens, that isn't procfs, it's usbfs. It's been plonked (*) on top of a procfs directory, but that doesn't matter.

Ciao,

Duncan.

(*) I believe the technical term is "mounted".

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Duncan Sands wrote:

|>Well perhaps I am on the wrong track but eg /proc/bus/usb/002/005 is my
|>digital camera and unless its either world rw or owned by me (user) I
|>can't get any pictures unless I make myself root.
|>
|>So yes, I would want to have chown/chmod in procfs ...
|
|
| Hi Clemens, that isn't procfs, it's usbfs.  It's been plonked (*) on

top of a procfs
| directory, but that doesn't matter.

total forgot about that :) Because if you don't compile the kernel with usbfs you don't see it there ...

- --
Clemens Schwaighofer - IT Engineer & System Administration
==========================================================
TEQUILA\Japan, 6-17-2 Ginza Chuo-ku, Tokyo 104-8167, JAPAN
Tel: +81-(0)3-3545-7703 Fax: +81-(0)3-3545-7343
http://www.tequila.co.jp
==========================================================
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA6Q3KjBz/yQjBxz8RArACAKDNr+SXs20yVvWdWBYGsbpCAkMqwACgqO2S srwaWJ2q6Wd0j5AOB4Y5UZM=
=kcN9
-----END PGP SIGNATURE-----

On a related note:

ahu@d800:/sys/devices/system/cpu/cpu0/cpufreq$ ls -l
total 0
-r--------    1 ahu      ahu          4096 2004-07-04 03:18 cpuinfo_cur_freq
-r--r--r--    1 ahu      ahu          4096 2004-07-04 03:18 cpuinfo_max_freq

..

I'm not entirely sure why the current CPU frequency has suddenly become a state secret, nor why the ownership of these files appears to be uid 1000.

-r--r--r--    1 ahu      ahu          4096 2004-07-04 03:18 cpuinfo_min_freq
-r--r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_available_frequencies
-r--r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_available_governors
-r--r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_cur_freq
-r--r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_driver
-rw-r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_governor
-rw-r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_max_freq
-rw-r--r--    1 ahu      ahu          4096 2004-07-04 03:18 scaling_min_freq

This appears to be even more bogus - uid 1000 can write to the scaling governor. It looks like the uid that mounted /sys owns these files. Not entirely sure what happens.

Fixes appear trivial but I wonder about the reasons.

Regards,

bert

http://www.PowerDNS.com      Open source, database driven DNS Software 
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO