Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

2005-07-26 Thread Rene Scharfe
On Tue, Jul 26, 2005 at 01:48:46PM -0400, Trond Myklebust wrote:
 I really don't like the choice of name. If you feel you must change the
 name, then make it something like nfs_blocksize_align(). That describes
 its function, instead of the implementation details.

Yes, rounddown_pow_of_two() belongs in kernel.h next to
roundup_pow_of_two().  And maybe it should get a shorter name.

Anyway, I also don't like nfs_blocksize_align.  So let's simply keep
the old name.  Renaming can be done later if really needed.

Rene


[PATCH 3/3] Simplify nfs_block_bits()

Signed-off-by: Rene Scharfe [EMAIL PROTECTED]
---

 fs/nfs/inode.c |   12 ++--
 1 files changed, 2 insertions(+), 10 deletions(-)

ddad5eadf4c2907842bf9baa2610e0a35ea14137
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -189,16 +189,8 @@ nfs_umount_begin(struct super_block *sb)
 static inline unsigned long
 nfs_block_bits(unsigned long bsize)
 {
-   /* make sure blocksize is a power of two */
-   if (bsize  (bsize - 1)) {
-   unsigned char   nrbits;
-
-   for (nrbits = 31; nrbits  !(bsize  (1  nrbits)); nrbits--)
-   ;
-   bsize = 1  nrbits;
-   }
-
-   return bsize;
+   /* round down to the nearest power of two */
+   return bsize ? (1UL  (fls(bsize) - 1)) : 0;
 }
 
 /*
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

2005-07-25 Thread Rene Scharfe
On Sun, Jul 24, 2005 at 07:24:23PM -0400, Trond Myklebust wrote:
 su den 24.07.2005 Klokka 19:09 (-0400) skreiv Trond Myklebust:
  What non-power-of-two target? Anything _not_ aligned to a power of two
  boundary is a BUG!

So we could simply replace the loop in nfs_block_bits() with call to
BUG()? :-

 Furthermore, rounding UP in order to correct this non-alignment would
 definitely be a bug.
 
 If users choose to override the default rsize/wsize, that is almost
 always in order to limit the UDP fragmentation per read/write request on
 lossy networks. By rounding up, you are doubling the number of fragments
 that the user requested instead of respecting the limit.

OK.  Either way, this function can be cleaned up.  Apparently the Plan 9
filesystem guys thought it rounds up.

What's your opinion of the following patch, which explicitly states the
intent of nfs_block_bits()?  It still needs patch 1 and 2.

Thanks,
Rene



[PATCH 3/4] Simplify and rename nfs_block_bits() to rounddown_pow_of_two()

nfs_block_bits() doesn't have a lot to do with bits anymore.  It can
also be implemented simpler and clearer with fls().

Signed-off-by: Rene Scharfe [EMAIL PROTECTED]
---

 fs/nfs/inode.c |   21 +
 1 files changed, 5 insertions(+), 16 deletions(-)

1388b63224334877b1b154e38ad9ee17f1726bca
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -185,20 +185,9 @@ nfs_umount_begin(struct super_block *sb)
rpc_killall_tasks(rpc);
 }
 
-
-static inline unsigned long
-nfs_block_bits(unsigned long bsize)
+static inline unsigned long rounddown_pow_of_two(unsigned long x)
 {
-   /* make sure blocksize is a power of two */
-   if (bsize  (bsize - 1)) {
-   unsigned char   nrbits;
-
-   for (nrbits = 31; nrbits  !(bsize  (1  nrbits)); nrbits--)
-   ;
-   bsize = 1  nrbits;
-   }
-
-   return bsize;
+   return x ? (1UL  (fls(x) - 1)) : 0;
 }
 
 /*
@@ -222,7 +211,7 @@ nfs_block_size(unsigned long bsize)
else if (bsize = NFS_MAX_FILE_IO_BUFFER_SIZE)
bsize = NFS_MAX_FILE_IO_BUFFER_SIZE;
 
-   return nfs_block_bits(bsize);
+   return rounddown_pow_of_two(bsize);
 }
 
 /*
@@ -319,10 +308,10 @@ nfs_sb_init(struct super_block *sb, rpc_
}
 
if (sb-s_blocksize == 0) {
-   sb-s_blocksize = nfs_block_bits(server-wsize);
+   sb-s_blocksize = rounddown_pow_of_two(server-wsize);
sb-s_blocksize_bits = fls(sb-s_blocksize - 1);
}
-   server-wtmult = nfs_block_bits(fsinfo.wtmult);
+   server-wtmult = rounddown_pow_of_two(fsinfo.wtmult);
 
server-dtsize = nfs_block_size(fsinfo.dtpref);
if (server-dtsize  PAGE_CACHE_SIZE)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH NFS 0/3] Cleanup/fix nfs_block_size() and nfs_block_bits()

2005-07-24 Thread Rene Scharfe
[PATCH NFS 0/3] Cleanup/fix nfs_block_size() and nfs_block_bits()

The Plan 9 filesystem for Linux had some code which wrongly calculated
the size of blocks.  A comment said this code has been taken from NFS.

Patch 3 of this series contains a fix for the bug, the first two patches
are preparation work and shouldn't change the results.  I didn't went so
far and actually test the patch, so please be careful. =)  Compiles fine
here, at least.

Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH NFS 1/3] Lose second parameter of nfs_block_size().

2005-07-24 Thread Rene Scharfe
[PATCH NFS 1/3] Lose second parameter of nfs_block_size().

Most calls to nfs_block_size() were done with a NULL pointer as second
parameter anyway.  We can simply calculate the number of bits ourselves
instead of using that ugly pointer thingy.

Signed-off-by: Rene Scharfe [EMAIL PROTECTED]
---

 fs/nfs/inode.c |   30 --
 1 files changed, 16 insertions(+), 14 deletions(-)

6ba4cb36d1e905852917305146fbe0076ad1d86f
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -217,14 +217,14 @@ nfs_calc_block_size(u64 tsize)
  * Compute and set NFS server blocksize
  */
 static inline unsigned long
-nfs_block_size(unsigned long bsize, unsigned char *nrbitsp)
+nfs_block_size(unsigned long bsize)
 {
if (bsize  1024)
bsize = NFS_DEF_FILE_IO_BUFFER_SIZE;
else if (bsize = NFS_MAX_FILE_IO_BUFFER_SIZE)
bsize = NFS_MAX_FILE_IO_BUFFER_SIZE;
 
-   return nfs_block_bits(bsize, nrbitsp);
+   return nfs_block_bits(bsize, NULL);
 }
 
 /*
@@ -293,16 +293,16 @@ nfs_sb_init(struct super_block *sb, rpc_
server-namelen = pathinfo.max_namelen;
/* Work out a lot of parameters */
if (server-rsize == 0)
-   server-rsize = nfs_block_size(fsinfo.rtpref, NULL);
+   server-rsize = nfs_block_size(fsinfo.rtpref);
if (server-wsize == 0)
-   server-wsize = nfs_block_size(fsinfo.wtpref, NULL);
+   server-wsize = nfs_block_size(fsinfo.wtpref);
 
if (fsinfo.rtmax = 512  server-rsize  fsinfo.rtmax)
-   server-rsize = nfs_block_size(fsinfo.rtmax, NULL);
+   server-rsize = nfs_block_size(fsinfo.rtmax);
if (fsinfo.wtmax = 512  server-wsize  fsinfo.wtmax)
-   server-wsize = nfs_block_size(fsinfo.wtmax, NULL);
+   server-wsize = nfs_block_size(fsinfo.wtmax);
 
-   max_rpc_payload = nfs_block_size(rpc_max_payload(server-client), NULL);
+   max_rpc_payload = nfs_block_size(rpc_max_payload(server-client));
if (server-rsize  max_rpc_payload)
server-rsize = max_rpc_payload;
if (server-wsize  max_rpc_payload)
@@ -325,7 +325,7 @@ nfs_sb_init(struct super_block *sb, rpc_
 sb-s_blocksize_bits);
server-wtmult = nfs_block_bits(fsinfo.wtmult, NULL);
 
-   server-dtsize = nfs_block_size(fsinfo.dtpref, NULL);
+   server-dtsize = nfs_block_size(fsinfo.dtpref);
if (server-dtsize  PAGE_CACHE_SIZE)
server-dtsize = PAGE_CACHE_SIZE;
if (server-dtsize  server-rsize)
@@ -419,12 +419,14 @@ nfs_fill_super(struct super_block *sb, s
server   = NFS_SB(sb);
sb-s_blocksize_bits = 0;
sb-s_blocksize = 0;
-   if (data-bsize)
-   sb-s_blocksize = nfs_block_size(data-bsize, 
sb-s_blocksize_bits);
+   if (data-bsize) {
+   sb-s_blocksize = nfs_block_size(data-bsize);
+   sb-s_blocksize_bits = fls(sb-s_blocksize - 1);
+   }
if (data-rsize)
-   server-rsize = nfs_block_size(data-rsize, NULL);
+   server-rsize = nfs_block_size(data-rsize);
if (data-wsize)
-   server-wsize = nfs_block_size(data-wsize, NULL);
+   server-wsize = nfs_block_size(data-wsize);
server-flags= data-flags  NFS_MOUNT_FLAGMASK;
 
server-acregmin = data-acregmin*HZ;
@@ -1619,9 +1621,9 @@ static int nfs4_fill_super(struct super_
sb-s_blocksize = 0;
server = NFS_SB(sb);
if (data-rsize != 0)
-   server-rsize = nfs_block_size(data-rsize, NULL);
+   server-rsize = nfs_block_size(data-rsize);
if (data-wsize != 0)
-   server-wsize = nfs_block_size(data-wsize, NULL);
+   server-wsize = nfs_block_size(data-wsize);
server-flags = data-flags  NFS_MOUNT_FLAGMASK;
server-caps = NFS_CAP_ATOMIC_OPEN;
 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

2005-07-24 Thread Rene Scharfe
[PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

Function nfs_block_bits() an open-coded version of (the non-existing)
rounddown_pow_of_two().  That means that for non-power-of-two target
sizes it returns half the size needed for a block to fully contain
the target.  I guess this is wrong. :-)  The patch uses the built-in
roundup_pow_of_two() instead.

Signed-off-by: Rene Scharfe [EMAIL PROTECTED]
---

 fs/nfs/inode.c |   22 +++---
 1 files changed, 3 insertions(+), 19 deletions(-)

4130722d1eeb5eb22c38df9f09dfa6be554bc72c
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -185,22 +185,6 @@ nfs_umount_begin(struct super_block *sb)
rpc_killall_tasks(rpc);
 }
 
-
-static inline unsigned long
-nfs_block_bits(unsigned long bsize)
-{
-   /* make sure blocksize is a power of two */
-   if (bsize  (bsize - 1)) {
-   unsigned char   nrbits;
-
-   for (nrbits = 31; nrbits  !(bsize  (1  nrbits)); nrbits--)
-   ;
-   bsize = 1  nrbits;
-   }
-
-   return bsize;
-}
-
 /*
  * Calculate the number of 512byte blocks used.
  */
@@ -222,7 +206,7 @@ nfs_block_size(unsigned long bsize)
else if (bsize = NFS_MAX_FILE_IO_BUFFER_SIZE)
bsize = NFS_MAX_FILE_IO_BUFFER_SIZE;
 
-   return nfs_block_bits(bsize);
+   return roundup_pow_of_two(bsize);
 }
 
 /*
@@ -319,10 +303,10 @@ nfs_sb_init(struct super_block *sb, rpc_
}
 
if (sb-s_blocksize == 0) {
-   sb-s_blocksize = nfs_block_bits(server-wsize);
+   sb-s_blocksize = roundup_pow_of_two(server-wsize);
sb-s_blocksize_bits = fls(sb-s_blocksize - 1);
}
-   server-wtmult = nfs_block_bits(fsinfo.wtmult);
+   server-wtmult = roundup_pow_of_two(fsinfo.wtmult);
 
server-dtsize = nfs_block_size(fsinfo.dtpref);
if (server-dtsize  PAGE_CACHE_SIZE)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH NFS 2/3] Lose second parameter of nfs_block_bits

2005-07-24 Thread Rene Scharfe
[PATCH NFS 2/3] Lose second parameter of nfs_block_bits

Two of the three calls were passing a NULL pointer and we can simply
calculate the number of bits ourselves.

Signed-off-by: Rene Scharfe [EMAIL PROTECTED]
---

 fs/nfs/inode.c |   17 -
 1 files changed, 8 insertions(+), 9 deletions(-)

d81155b5789ac9d7e05261f5f4bf639e7982fa4b
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -187,17 +187,15 @@ nfs_umount_begin(struct super_block *sb)
 
 
 static inline unsigned long
-nfs_block_bits(unsigned long bsize, unsigned char *nrbitsp)
+nfs_block_bits(unsigned long bsize)
 {
/* make sure blocksize is a power of two */
-   if ((bsize  (bsize - 1)) || nrbitsp) {
+   if (bsize  (bsize - 1)) {
unsigned char   nrbits;
 
for (nrbits = 31; nrbits  !(bsize  (1  nrbits)); nrbits--)
;
bsize = 1  nrbits;
-   if (nrbitsp)
-   *nrbitsp = nrbits;
}
 
return bsize;
@@ -224,7 +222,7 @@ nfs_block_size(unsigned long bsize)
else if (bsize = NFS_MAX_FILE_IO_BUFFER_SIZE)
bsize = NFS_MAX_FILE_IO_BUFFER_SIZE;
 
-   return nfs_block_bits(bsize, NULL);
+   return nfs_block_bits(bsize);
 }
 
 /*
@@ -320,10 +318,11 @@ nfs_sb_init(struct super_block *sb, rpc_
 server-wsize = server-wpages  PAGE_CACHE_SHIFT;
}
 
-   if (sb-s_blocksize == 0)
-   sb-s_blocksize = nfs_block_bits(server-wsize,
-sb-s_blocksize_bits);
-   server-wtmult = nfs_block_bits(fsinfo.wtmult, NULL);
+   if (sb-s_blocksize == 0) {
+   sb-s_blocksize = nfs_block_bits(server-wsize);
+   sb-s_blocksize_bits = fls(sb-s_blocksize - 1);
+   }
+   server-wtmult = nfs_block_bits(fsinfo.wtmult);
 
server-dtsize = nfs_block_size(fsinfo.dtpref);
if (server-dtsize  PAGE_CACHE_SIZE)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 2/7] procfs privacy: tasks/processes lookup

2005-04-20 Thread Rene Scharfe
Lorenzo Hernández García-Hierro schrieb:
 This patch restricts non-root users to view only their own processes.

You may also want to have a look at the patches I submitted over the
last few weeks that restricted some file permissions in /proc/pid/ and
the comments I received.

Regards,
Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [RFC][PATCH] Simple privacy enhancement for /proc/pid

2005-04-12 Thread Rene Scharfe
On Tue, Apr 12, 2005 at 01:29:35AM -0400, Albert Cahalan wrote:
 If you really can't allow access based on tty, then at least allow
 access if any UID value matches any UID value. Without this, a user
 can not always see a setuid program they are running.

Yes, that's a bug.  Below is a new version of the patch that keeps
setuid'd processes visible to any user.

 This means mucking with boot parameters, which can be a pain.
 The various boot loaders do not all use the same config file.

True, kernel parameters not as comfortable to use as sysctl's or mount
options.  You only have to set it once, however.

  Then I suppose we don't need to be able to fine-tune the permissions for
  each file in /proc/pid/.  All that we need is a distinction between
  normal users (which are to be restricted) and admins (which need to
  see everything).
 
 The /proc/*/maps file sure is different from the /proc/*/status file.
 The same for all the others, really.

The contents and purposes are different, but the intent of my patch is
to protect all information about the processes of one user from all
others.  If you need to access that information you have at least three
options: a) don't enable proc.privacy, b) put the users who need the
info into the proc.gid group, c) chmod a+r /proc/$pid/$file.  (Note:
that last option is a feature of the vanilla kernel.)

  This patch introduces two kernel parameters: proc.privacy and proc.gid.
  The group ID attribute of all files below /proc/pid is set to
  proc.gid, but only if you activate the feature by setting proc.privacy
  to a non-zero value.
 
 This is very bad. Please do not change the GID as seen by
 the stat() call. This value is used.

What is it used for?  E.g. using the group ID of /proc/pid/stat to
determine the egid that process is running as is not reliable even
without my patch.  It's better to use the group ID of the directory
/proc/pid/ instead or to look up the ID in /proc/pid/status.  And
the permissions of the pid directories are not changed by the patch.

Thanks,
Rene


diff -pur l1/Documentation/kernel-parameters.txt 
l2/Documentation/kernel-parameters.txt
--- l1/Documentation/kernel-parameters.txt  2005-04-07 23:18:36.0 
+0200
+++ l2/Documentation/kernel-parameters.txt  2005-04-10 16:56:58.0 
+0200
@@ -1116,16 +1116,27 @@ running once the system is up.
[ISAPNP] Exclude memory regions for the 
autoconfiguration
Ranges are in pairs (memory base and size).
 
+   processor.max_cstate=   [HW, ACPI]
+   Limit processor to maximum C-state
+   max_cstate=9 overrides any DMI blacklist limit.
+
+   proc.gid=   [KNL] Group ID attribute of the files in /proc/pid,
+   default is 0.  This parameter is ignored if
+   proc.privacy is 0.
+   proc.privacy=   [KNL] Take away permissions for files in /proc/pid
+   from world (think chmod o-rwx) and apply proc.gid
+   setting.  0 = disabled (default), 1 = restrict access
+   to all process details except those having a uid of 0,
+   2 = restrict access to all process details except for
+   kernel threads and init and its children, 3 = restrict
+   access to all process details.
+
profile=[KNL] Enable kernel profiling via /proc/profile
{ schedule | number }
(param: schedule - profile schedule points}
(param: profile step/bucket size as a power of 2 for
statistical time based profiling)
 
-   processor.max_cstate=   [HW, ACPI]
-   Limit processor to maximum C-state
-   max_cstate=9 overrides any DMI blacklist limit.
-
prompt_ramdisk= [RAM] List of RAM disks to prompt for floppy disk
before loading.
See Documentation/ramdisk.txt.
diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-04-07 23:18:47.0 +0200
+++ l2/fs/proc/base.c   2005-04-12 21:26:13.0 +0200
@@ -35,8 +35,18 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/module.h
+#include linux/moduleparam.h
 #include internal.h
 
+static int proc_privacy;
+module_param_named(privacy, proc_privacy, uint, 0);
+MODULE_PARM_DESC(umask, restrict permissions of files in /proc/pid);
+
+static gid_t proc_gid;
+module_param_named(gid, proc_gid, uint, 0);
+MODULE_PARM_DESC(gid, process admin group);
+
 /*
  * For hysterical raisins we keep the same inumbers as in the old procfs.
  * Feel free to change the macro below - just keep the range distinct from
@@ -227,6 +237,8 @@ static struct pid_entry tid_attr_stuff[]
 
 #undef E
 
+#define IS_PID_DIR(type) ((type) == PROC_TGID_INO || (type) == PROC_TID_INO)
+
 static 

Re: [RFC][PATCH] Simple privacy enhancement for /proc/pid

2005-04-11 Thread Rene Scharfe
Bodo Eggert schrieb:
 On Sun, 10 Apr 2005, Rene Scharfe wrote:
 
 
First, configuring via kernel parameters is sufficient.
 
 
 I don't remember: Would a mount option be equally easy to implement?
 (Kernel parameters are OK for me, too.)

A mount option for procfs would be changable at remount, making
implementation a bit more involved.

I have another idea: let's keep the details of _every_ process owned by
user root readable by anyone.
 
 
 What about SUID processes acting on behalf of users?

SUID root processes will we visible for all, too.  That's fair enough, I
think.  If it's a concern to you use proc.privacy=2.

- processor.max_cstate=   [HW, ACPI]
- Limit processor to maximum C-state
- max_cstate=9 overrides any DMI blacklist limit.
-
 
 
 This seems to belong into another patch

Strictly speaking, yes, but it's just a trivial cleanup near my own
change.  And I guarantee it has zero impact on any built kernel image. :]

 (in pid_revalidate:)
 What about moving the things around? (just editing in the MUA)
 
 
+ if (IS_PID_DIR(proc_type(inode)) || task_dumpable(task)) {
  inode-i_uid = task-euid;
+ inode-i_gid = proc_gid;
+ if (!proc_privacy || IS_PID_DIR(proc_type(inode)))
  inode-i_gid = task-egid;
  } else {
  inode-i_uid = 0;
  inode-i_gid = 0;
  }
  security_task_to_inode(task, inode);
  return 1;
  }

I suppose you could do that, but I don't see any gain.  I also find my
version easier to read because it keeps the two conditionals (having
different intents and purposes) apart.

 BTW: You might be able to cache IS_PID_DIR(). It looks like being a gain.

I'd rather let the compiler do that job.  It's only a small macro, I
really doubt you would measure any speedup from putting it into a local
variable.

@@ -1454,6 +1468,11 @@ static struct dentry *proc_pident_lookup
 
 
+ if (proc_privacy == 2 || task-euid != 0)
 
^
 redundand.

You're right and it's a matter of taste, I guess.  By the way, this is
also what the FreeBSD crowd calls a bikeshed. :-)

Thanks for reviewing my patch!
Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[RFC][PATCH] Simple privacy enhancement for /proc/pid

2005-04-10 Thread Rene Scharfe
Hi all,

sorry it took me so long before offering another patch for restricting
/proc permissions.  Real life kept on intervening.

Albert, allowing access based on tty sounds nice, but it _is_ expansive.
More importantly, perhaps, it would virtualize /proc: every user would
see different permissions for certain files in there.  That's too comlex
for my taste.

My previous patchset was complex, too, given its simple purpose: to
restrict regular users from spying on each other.  So let's cut out what
we don't really need.

First, configuring via kernel parameters is sufficient.  It simplifies
implementation a lot because we know the settings cannot change.  And we
don't need the added flexibility of sysctls anyway -- I assume these
parameters are set at installation time and never touched again.

Then I suppose we don't need to be able to fine-tune the permissions for
each file in /proc/pid/.  All that we need is a distinction between
normal users (which are to be restricted) and admins (which need to
see everything).

In a previous mail I asked how to identify tasks every user should be
able to see details of.  Bodo came up with some nice ideas, like
checking if parent is init(1) to catch daemons and identifying kernel
threads by their unique ability to block SIGKILL.  That's simple and
catches most interesting processes; it fails to catch the children of
forking servers like apache and, notably, sshd, however.

I have another idea: let's keep the details of _every_ process owned by
user root readable by anyone.  The superuser doesn't deserve privacy.
No new hole is opened, as root's files don't change their permissions.
Admittedly, admins are people, too, and they can get privacy, too -- but
only if they use their own regular user ID.  They should be doing that
anyway.

This catches the _important_ processes, those used to provide logins
(and then some :-).  Tools like w, who and pstree keep on working just
fine, even for SSH logins.

This patch introduces two kernel parameters: proc.privacy and proc.gid.
The group ID attribute of all files below /proc/pid is set to
proc.gid, but only if you activate the feature by setting proc.privacy
to a non-zero value.  1 means to allow all users to see root's process
details and hide everyone else's (as described above), 2 gives you the
shared nothing semantics where even root's processes are cloaked.

Patch is against 2.6.12-rc2-mm2, please give it a try and tell me how
you like it.

Thanks,
Rene



diff -pur l1/Documentation/kernel-parameters.txt 
l2/Documentation/kernel-parameters.txt
--- l1/Documentation/kernel-parameters.txt  2005-04-07 23:18:36.0 
+0200
+++ l2/Documentation/kernel-parameters.txt  2005-04-10 13:52:56.0 
+0200
@@ -1116,16 +1116,25 @@ running once the system is up.
[ISAPNP] Exclude memory regions for the 
autoconfiguration
Ranges are in pairs (memory base and size).
 
+   processor.max_cstate=   [HW, ACPI]
+   Limit processor to maximum C-state
+   max_cstate=9 overrides any DMI blacklist limit.
+
+   proc.gid=   [KNL] Group ID attribute of the files in /proc/pid,
+   default is 0.  This parameter is ignored if
+   proc.privacy is 0.
+   proc.privacy=   [KNL] Take away permissions for files in /proc/pid
+   from world (think chmod o-rwx) and apply proc.gid
+   setting.  0 = disabled (default), 1 = restrict access
+   to all process details except those having a uid of 0,
+   2 = restrict access to all process details.
+
profile=[KNL] Enable kernel profiling via /proc/profile
{ schedule | number }
(param: schedule - profile schedule points}
(param: profile step/bucket size as a power of 2 for
statistical time based profiling)
 
-   processor.max_cstate=   [HW, ACPI]
-   Limit processor to maximum C-state
-   max_cstate=9 overrides any DMI blacklist limit.
-
prompt_ramdisk= [RAM] List of RAM disks to prompt for floppy disk
before loading.
See Documentation/ramdisk.txt.
diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-04-07 23:18:47.0 +0200
+++ l2/fs/proc/base.c   2005-04-10 14:00:28.0 +0200
@@ -35,8 +35,18 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/module.h
+#include linux/moduleparam.h
 #include internal.h
 
+static int proc_privacy;
+module_param_named(privacy, proc_privacy, uint, 0);
+MODULE_PARM_DESC(umask, restrict permissions of files in /proc/pid);
+
+static gid_t proc_gid;
+module_param_named(gid, proc_gid, uint, 0);
+MODULE_PARM_DESC(gid, process admin group);
+
 /*
  * For hysterical 

[PATCH][2/5] Sysctl for proc

2005-03-20 Thread Rene Scharfe
Add an array to store file permission settings in and add an example
sysctl (proc.cmdline).  It can already be set and read, but it has no
effect, yet.

diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-03-19 01:28:47.0 +0100
+++ l2/fs/proc/base.c   2005-03-19 19:50:37.0 +0100
@@ -227,6 +227,13 @@ static struct pid_entry tid_attr_stuff[]
 
 #undef E
 
+#ifdef CONFIG_SYSCTL
+/* Order and number of elements must match CTL_PROC table in sysctl.h! */
+mode_t proc_base_permissions[] = {
+   S_IRUGO,/* PROC_CMDLINE */
+};
+#endif /* CONFIG_SYSCTL */
+
 static int proc_fd_link(struct inode *inode, struct dentry **dentry, struct 
vfsmount **mnt)
 {
struct task_struct *task = proc_task(inode);
diff -pur l1/include/linux/sysctl.h l2/include/linux/sysctl.h
--- l1/include/linux/sysctl.h   2005-03-19 19:49:43.0 +0100
+++ l2/include/linux/sysctl.h   2005-03-19 19:49:53.0 +0100
@@ -654,6 +654,9 @@ enum {
 };
 
 /* CTL_PROC names: */
+enum {
+   PROC_CMDLINE= 1,
+};
 
 /* CTL_FS names: */
 enum
diff -pur l1/kernel/sysctl.c l2/kernel/sysctl.c
--- l1/kernel/sysctl.c  2005-03-19 19:49:43.0 +0100
+++ l2/kernel/sysctl.c  2005-03-19 19:49:53.0 +0100
@@ -168,6 +168,10 @@ extern struct proc_dir_entry *proc_sys_r
 
 static void register_proc_table(ctl_table *, struct proc_dir_entry *);
 static void unregister_proc_table(ctl_table *, struct proc_dir_entry *);
+
+extern mode_t proc_base_permissions[];
+
+static int mode_r_ugo = S_IRUGO;
 #endif
 
 /* The default sysctl tables: */
@@ -840,6 +844,16 @@ static ctl_table vm_table[] = {
 };
 
 static ctl_table proc_table[] = {
+#ifdef CONFIG_PROC_FS
+   {
+   .ctl_name   = PROC_CMDLINE,
+   .procname   = cmdline,
+   .data   = proc_base_permissions[PROC_CMDLINE-1],
+   .mode   = 0644,
+   .proc_handler   = proc_domode,
+   .extra1 = mode_r_ugo,
+   },
+#endif
{ .ctl_name = 0 }
 };
 

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][5/5] Four more sysctls

2005-03-20 Thread Rene Scharfe
Add four more sysctls: proc.maps, proc.stat, proc.statm, proc.status.

diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-03-19 20:10:22.0 +0100
+++ l2/fs/proc/base.c   2005-03-19 20:11:38.0 +0100
@@ -149,11 +149,11 @@ static struct pid_entry tgid_base_stuff[
E(PROC_TGID_FD,fd,  S_IFDIR|S_IRUSR|S_IXUSR),
E(PROC_TGID_ENVIRON,   environ, S_IFREG|S_IRUSR),
E(PROC_TGID_AUXV,  auxv,S_IFREG|S_IRUSR),
-   E(PROC_TGID_STATUS,status,  S_IFREG|S_IRUGO),
+   S(PROC_TGID_STATUS,status,  S_IFREG|S_IRUGO, PROC_STATUS),
S(PROC_TGID_CMDLINE,   cmdline, S_IFREG|S_IRUGO, PROC_CMDLINE),
-   E(PROC_TGID_STAT,  stat,S_IFREG|S_IRUGO),
-   E(PROC_TGID_STATM, statm,   S_IFREG|S_IRUGO),
-   E(PROC_TGID_MAPS,  maps,S_IFREG|S_IRUGO),
+   S(PROC_TGID_STAT,  stat,S_IFREG|S_IRUGO, PROC_STAT),
+   S(PROC_TGID_STATM, statm,   S_IFREG|S_IRUGO, PROC_STATM),
+   S(PROC_TGID_MAPS,  maps,S_IFREG|S_IRUGO, PROC_MAPS),
E(PROC_TGID_MEM,   mem, S_IFREG|S_IRUSR|S_IWUSR),
 #ifdef CONFIG_SECCOMP
E(PROC_TGID_SECCOMP,   seccomp, S_IFREG|S_IRUSR|S_IWUSR),
@@ -185,11 +185,11 @@ static struct pid_entry tid_base_stuff[]
E(PROC_TID_FD, fd,  S_IFDIR|S_IRUSR|S_IXUSR),
E(PROC_TID_ENVIRON,environ, S_IFREG|S_IRUSR),
E(PROC_TID_AUXV,   auxv,S_IFREG|S_IRUSR),
-   E(PROC_TID_STATUS, status,  S_IFREG|S_IRUGO),
+   S(PROC_TID_STATUS, status,  S_IFREG|S_IRUGO, PROC_STATUS),
S(PROC_TID_CMDLINE,cmdline, S_IFREG|S_IRUGO, PROC_CMDLINE),
-   E(PROC_TID_STAT,   stat,S_IFREG|S_IRUGO),
-   E(PROC_TID_STATM,  statm,   S_IFREG|S_IRUGO),
-   E(PROC_TID_MAPS,   maps,S_IFREG|S_IRUGO),
+   S(PROC_TID_STAT,   stat,S_IFREG|S_IRUGO, PROC_STAT),
+   S(PROC_TID_STATM,  statm,   S_IFREG|S_IRUGO, PROC_STATM),
+   S(PROC_TID_MAPS,   maps,S_IFREG|S_IRUGO, PROC_MAPS),
E(PROC_TID_MEM,mem, S_IFREG|S_IRUSR|S_IWUSR),
 #ifdef CONFIG_SECCOMP
E(PROC_TID_SECCOMP,seccomp, S_IFREG|S_IRUSR|S_IWUSR),
@@ -242,6 +242,10 @@ static struct pid_entry tid_attr_stuff[]
 /* Order and number of elements must match CTL_PROC table in sysctl.h! */
 mode_t proc_base_permissions[] = {
S_IRUGO,/* PROC_CMDLINE */
+   S_IRUGO,/* PROC_MAPS */
+   S_IRUGO,/* PROC_STAT */
+   S_IRUGO,/* PROC_STATM */
+   S_IRUGO,/* PROC_STATUS */
 };
 
 static void proc_update_mode(struct inode *inode)
diff -pur l1/include/linux/sysctl.h l2/include/linux/sysctl.h
--- l1/include/linux/sysctl.h   2005-03-19 20:08:27.0 +0100
+++ l2/include/linux/sysctl.h   2005-03-19 20:10:31.0 +0100
@@ -656,6 +656,10 @@ enum {
 /* CTL_PROC names: */
 enum {
PROC_CMDLINE= 1,
+   PROC_MAPS   = 2,
+   PROC_STAT   = 3,
+   PROC_STATM  = 4,
+   PROC_STATUS = 5,
 };
 
 /* CTL_FS names: */
diff -pur l1/kernel/sysctl.c l2/kernel/sysctl.c
--- l1/kernel/sysctl.c  2005-03-19 20:08:27.0 +0100
+++ l2/kernel/sysctl.c  2005-03-19 20:10:31.0 +0100
@@ -853,6 +853,38 @@ static ctl_table proc_table[] = {
.proc_handler   = proc_domode,
.extra1 = mode_r_ugo,
},
+   {
+   .ctl_name   = PROC_MAPS,
+   .procname   = maps,
+   .data   = proc_base_permissions[PROC_MAPS-1],
+   .mode   = 0644,
+   .proc_handler   = proc_domode,
+   .extra1 = mode_r_ugo,
+   },
+   {
+   .ctl_name   = PROC_STAT,
+   .procname   = stat,
+   .data   = proc_base_permissions[PROC_STAT-1],
+   .mode   = 0644,
+   .proc_handler   = proc_domode,
+   .extra1 = mode_r_ugo,
+   },
+   {
+   .ctl_name   = PROC_STATM,
+   .procname   = statm,
+   .data   = proc_base_permissions[PROC_STATM-1],
+   .mode   = 0644,
+   .proc_handler   = proc_domode,
+   .extra1 = mode_r_ugo,
+   },
+   {
+   .ctl_name   = PROC_STATUS,
+   .procname   = status,
+   .data   = proc_base_permissions[PROC_STATUS-1],
+   .mode   = 0644,
+   .proc_handler   = proc_domode,
+   .extra1 = mode_r_ugo,
+   },
 #endif
{ .ctl_name = 0 }
 };

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][4/5] Add inode_operations for proc sysctl

2005-03-20 Thread Rene Scharfe
Add getattr and permission inode_operations for base /proc/pid entries
that make sure the value of inode-i_mode is up-to-date with its
respective sysctl setting.  This is achieved by calling the new function
proc_update_mode.  It currently spares the init process.

diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-03-19 20:08:30.0 +0100
+++ l2/fs/proc/base.c   2005-03-19 20:08:39.0 +0100
@@ -243,6 +243,40 @@ static struct pid_entry tid_attr_stuff[]
 mode_t proc_base_permissions[] = {
S_IRUGO,/* PROC_CMDLINE */
 };
+
+static void proc_update_mode(struct inode *inode)
+{
+   struct task_struct *task = proc_task(inode);
+   int ctl_name = proc_ctl_name(inode);
+
+   if (ctl_name == 0)
+   return;
+   if (task-pid == 1) /* Don't touch init.  TODO: kernel threads? */
+   return;
+   inode-i_mode = (inode-i_mode  ~S_IALLUGO) |
+   proc_base_permissions[ctl_name-1];
+}
+
+static int proc_base_permission(struct inode *inode, int mask,
+struct nameidata *nd)
+{
+   proc_update_mode(inode);
+   return generic_permission(inode, mask, NULL);
+}
+
+static int proc_base_getattr(struct vfsmount *mnt, struct dentry *dentry,
+ struct kstat *stat)
+{
+   struct inode *inode = dentry-d_inode;
+   proc_update_mode(inode);
+   generic_fillattr(inode, stat);
+   return 0;
+}
+
+static struct inode_operations proc_base_inode_operations = {
+   .getattr= proc_base_getattr,
+   .permission = proc_base_permission,
+};
 #endif /* CONFIG_SYSCTL */
 
 static int proc_fd_link(struct inode *inode, struct dentry **dentry, struct 
vfsmount **mnt)
@@ -1474,6 +1508,8 @@ static struct dentry *proc_pident_lookup
ei = PROC_I(inode);
 #ifdef CONFIG_SYSCTL
ei-ctl_name = p-ctl_name;
+   proc_update_mode(inode);
+   inode-i_op = proc_base_inode_operations;
 #endif
inode-i_mode = p-mode;
/*

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][3/5] New member for proc_inode: ctl_name

2005-03-20 Thread Rene Scharfe
Add new field to struct proc_inode and struct pid_entry: ctl_name.  It
will be used to hold the ctl_name value of the sysctl that is responsible
for the respective inode or pid_entry.  Also initialize this value for
our example sysctl (proc.cmdline).

diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-03-19 19:59:00.0 +0100
+++ l2/fs/proc/base.c   2005-03-19 19:59:43.0 +0100
@@ -35,6 +35,7 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/sysctl.h
 #include internal.h
 
 /*
@@ -130,9 +131,18 @@ struct pid_entry {
int len;
char *name;
mode_t mode;
+#ifdef CONFIG_SYSCTL
+   int ctl_name;
+#endif
 };
 
-#define E(type,name,mode) {(type),sizeof(name)-1,(name),(mode)}
+#ifdef CONFIG_SYSCTL
+#define E(type, name, mode){(type), sizeof(name)-1, (name), 
(mode), 0}
+#define S(type, name, mode, ctl_name)  {(type), sizeof(name)-1, (name), 
(mode), (ctl_name)}
+#else
+#define E(type, name, mode){(type), sizeof(name)-1, (name), (mode)}
+#define S(type, name, mode, ctl_name)  {(type), sizeof(name)-1, (name), (mode)}
+#endif /* CONFIG_SYSCTL */
 
 static struct pid_entry tgid_base_stuff[] = {
E(PROC_TGID_TASK,  task,S_IFDIR|S_IRUGO|S_IXUGO),
@@ -140,7 +150,7 @@ static struct pid_entry tgid_base_stuff[
E(PROC_TGID_ENVIRON,   environ, S_IFREG|S_IRUSR),
E(PROC_TGID_AUXV,  auxv,S_IFREG|S_IRUSR),
E(PROC_TGID_STATUS,status,  S_IFREG|S_IRUGO),
-   E(PROC_TGID_CMDLINE,   cmdline, S_IFREG|S_IRUGO),
+   S(PROC_TGID_CMDLINE,   cmdline, S_IFREG|S_IRUGO, PROC_CMDLINE),
E(PROC_TGID_STAT,  stat,S_IFREG|S_IRUGO),
E(PROC_TGID_STATM, statm,   S_IFREG|S_IRUGO),
E(PROC_TGID_MAPS,  maps,S_IFREG|S_IRUGO),
@@ -176,7 +186,7 @@ static struct pid_entry tid_base_stuff[]
E(PROC_TID_ENVIRON,environ, S_IFREG|S_IRUSR),
E(PROC_TID_AUXV,   auxv,S_IFREG|S_IRUSR),
E(PROC_TID_STATUS, status,  S_IFREG|S_IRUGO),
-   E(PROC_TID_CMDLINE,cmdline, S_IFREG|S_IRUGO),
+   S(PROC_TID_CMDLINE,cmdline, S_IFREG|S_IRUGO, PROC_CMDLINE),
E(PROC_TID_STAT,   stat,S_IFREG|S_IRUGO),
E(PROC_TID_STATM,  statm,   S_IFREG|S_IRUGO),
E(PROC_TID_MAPS,   maps,S_IFREG|S_IRUGO),
@@ -226,6 +236,7 @@ static struct pid_entry tid_attr_stuff[]
 #endif
 
 #undef E
+#undef S
 
 #ifdef CONFIG_SYSCTL
 /* Order and number of elements must match CTL_PROC table in sysctl.h! */
@@ -1150,6 +1161,9 @@ static struct inode *proc_pid_make_inode
get_task_struct(task);
ei-task = task;
ei-type = ino;
+#ifdef CONFIG_SYSCTL
+   ei-ctl_name = 0;
+#endif
inode-i_uid = 0;
inode-i_gid = 0;
if (ino == PROC_TGID_INO || ino == PROC_TID_INO || task_dumpable(task)) 
{
@@ -1458,6 +1472,9 @@ static struct dentry *proc_pident_lookup
goto out;
 
ei = PROC_I(inode);
+#ifdef CONFIG_SYSCTL
+   ei-ctl_name = p-ctl_name;
+#endif
inode-i_mode = p-mode;
/*
 * Yes, it does not scale. And it should not. Don't add
diff -pur l1/fs/proc/internal.h l2/fs/proc/internal.h
--- l1/fs/proc/internal.h   2005-02-12 09:26:31.0 +0100
+++ l2/fs/proc/internal.h   2005-03-19 19:55:44.0 +0100
@@ -46,3 +46,8 @@ static inline int proc_type(struct inode
 {
return PROC_I(inode)-type;
 }
+
+static inline int proc_ctl_name(struct inode *inode)
+{
+   return PROC_I(inode)-ctl_name;
+}
diff -pur l1/include/linux/proc_fs.h l2/include/linux/proc_fs.h
--- l1/include/linux/proc_fs.h  2005-02-12 09:26:36.0 +0100
+++ l2/include/linux/proc_fs.h  2005-03-19 19:55:44.0 +0100
@@ -238,6 +238,7 @@ extern void kclist_add(struct kcore_list
 struct proc_inode {
struct task_struct *task;
int type;
+   int ctl_name;
union {
int (*proc_get_link)(struct inode *, struct dentry **, struct 
vfsmount **);
int (*proc_read)(struct task_struct *task, char *page);

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][1/5] Introduce proc_domode

2005-03-20 Thread Rene Scharfe
[The patches seem to not have made it to the list the first time,
I'm retrying without CC:s.]

Add a new sysctl proc handler , proc_domode.  It can be used to implement
sysctls for setting and/or displaying file mode or file mask values.

diff -pur l1/include/linux/sysctl.h l2/include/linux/sysctl.h
--- l1/include/linux/sysctl.h   2005-03-19 01:28:48.0 +0100
+++ l2/include/linux/sysctl.h   2005-03-19 08:08:43.0 +0100
@@ -812,6 +812,8 @@ extern int proc_doulonglongvec_minmax(ct
  void __user *, size_t *, loff_t *);
 extern int proc_doulongvec_ms_jiffies_minmax(ctl_table *table, int,
  struct file *, void __user *, size_t *, 
loff_t *);
+extern int proc_domode(ctl_table *, int, struct file *,
+   void __user *, size_t *, loff_t *);
 
 extern int do_sysctl (int __user *name, int nlen,
  void __user *oldval, size_t __user *oldlenp,
diff -pur l1/kernel/sysctl.c l2/kernel/sysctl.c
--- l1/kernel/sysctl.c  2005-03-19 01:28:49.0 +0100
+++ l2/kernel/sysctl.c  2005-03-19 08:16:41.0 +0100
@@ -2120,6 +2120,81 @@ int proc_dointvec_ms_jiffies(ctl_table *
do_proc_dointvec_ms_jiffies_conv, NULL);
 }
 
+/**
+ * proc_domode - read a file mode value
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @filp: the file structure
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes one file mode value (data type mode_t) 
+ * from/to the user buffer, treated as an ASCII string. 
+ * Optionally checks that the value fits within the mask
+ * specified with *table-extra1.  That mask cannot be
+ * bigger than 0.
+ *
+ * Returns 0 on success.
+ */
+int proc_domode(ctl_table *table, int write, struct file *filp,
+   void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+#define TMPBUFLEN  6
+#define MAXMODE0
+   size_t len;
+   char __user *p;
+   char c;
+   char buf[TMPBUFLEN];
+   char *endp;
+   mode_t maxmask = MAXMODE;
+   unsigned long mode;
+   
+   if (!table-data || !*lenp || (*ppos  !write)) {
+   *lenp = 0;
+   return 0;
+   }
+   
+   if (write) {
+   if (table-extra1)
+   maxmask = *((mode_t *)table-extra1);
+   len = 0;
+   p = buffer;
+   while (len  *lenp) {
+   if (get_user(c, p++))
+   return -EFAULT;
+   if (c == 0 || c == '\n')
+   break;
+   len++;
+   }
+   if (len  TMPBUFLEN)
+   return -EINVAL;
+   if (copy_from_user(buf, buffer, len))
+   return -EFAULT;
+   buf[len] = '\0';
+   mode = simple_strtoul(buf, endp, 0);
+   if (mode  ~maxmask)
+   return -EPERM;
+   *((mode_t *)table-data) = mode;
+   *ppos += *lenp;
+   } else {
+   if (*((mode_t *)table-data)  MAXMODE)
+   return -EINVAL;
+   mode = *((mode_t *)table-data);
+   len = sprintf(buf, 0%03o\n, (mode_t)mode);
+   if (len  *lenp)
+   len = *lenp;
+   if (len)
+   if (copy_to_user(buffer, buf, len))
+   return -EFAULT;
+   *lenp = len;
+   *ppos += len;
+   }
+   return 0;
+#undef TMPBUFLEN
+#undef MAXMODE
+}
+
 #else /* CONFIG_PROC_FS */
 
 int proc_dostring(ctl_table *table, int write, struct file *filp,
@@ -2191,6 +2266,12 @@ int proc_doulongvec_ms_jiffies_minmax(ct
 return -ENOSYS;
 }
 
+int proc_domode(ctl_table *table, int write, struct file *filp,
+   void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+   return -ENOSYS;
+}
+
 
 #endif /* CONFIG_PROC_FS */
 
@@ -2432,6 +2513,12 @@ int proc_doulongvec_ms_jiffies_minmax(ct
 return -ENOSYS;
 }
 
+int proc_domode(ctl_table *table, int write, struct file *filp,
+   void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+   return -ENOSYS;
+}
+
 struct ctl_table_header * register_sysctl_table(ctl_table * table, 
int insert_at_head)
 {
@@ -2453,6 +2540,7 @@ EXPORT_SYMBOL(proc_dointvec_jiffies);
 EXPORT_SYMBOL(proc_dointvec_minmax);
 EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
 EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
+EXPORT_SYMBOL(proc_domode);
 EXPORT_SYMBOL(proc_dostring);
 EXPORT_SYMBOL(proc_doulongvec_minmax);
 EXPORT_SYMBOL(proc_doulonglongvec_minmax);

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  

[PATCH][6/5] Bonus: unrelated minor cleanup of enum pid_directory_inos

2005-03-20 Thread Rene Scharfe
Cleanup: remove PROC_TGID_FD_DIR because it's unused.  Also put
PROC_TID_FD_DIR at the end of the enum to avoid clashing of the FD
range (as noted in the comment) with PROC_TID_OOM_SCORE and
PROC_TID_OOM_ADJUST.  It's not a _problem_, because FD links and
these OOM related files are in different directories.  It is
certainly untidy.

diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-03-19 22:16:48.0 +0100
+++ l2/fs/proc/base.c   2005-03-19 22:16:59.0 +0100
@@ -84,7 +84,6 @@ enum pid_directory_inos {
 #ifdef CONFIG_AUDITSYSCALL
PROC_TGID_LOGINUID,
 #endif
-   PROC_TGID_FD_DIR,
PROC_TGID_OOM_SCORE,
PROC_TGID_OOM_ADJUST,
PROC_TID_INO,
@@ -121,9 +120,9 @@ enum pid_directory_inos {
 #ifdef CONFIG_AUDITSYSCALL
PROC_TID_LOGINUID,
 #endif
-   PROC_TID_FD_DIR = 0x8000,   /* 0x8000-0x */
PROC_TID_OOM_SCORE,
PROC_TID_OOM_ADJUST,
+   PROC_TID_FD_DIR = 0x8000,   /* 0x8000-0x */
 };
 
 struct pid_entry {

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH][1/5] Introduce proc_domode

2005-03-20 Thread Rene Scharfe
Jan Engelhardt wrote:
+#define MAXMODE		0

I think this should be 0777. SUID, SGID and sticky bit do not belong
into /proc.
It's not /proc specific, the function proc_domode can be used for all
kinds of sysctls.  It enforces a maximum mode, specified in the -extra1
member of a sysctl table entry.  In patches 2 and 5 you can see this
value is 0444 for all five new sysctls.
Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][0/6] Change proc file permissions with sysctls

2005-03-19 Thread Rene Scharfe
The following patches implement another interface that allows an admin
to restrict permissions inside /proc/pid to enhance the privacy of
users.  Following a suggestion by Albert Calahan this set of patches
introduces five sysctls, each one changes the permissions of a certain
file in /proc/pid.

It works by implementing getattr and permission methods that update the
files' permissions (btw. Al Viro suggested doing it this way right from
the start..).

To cloak as much as currently possible:

   # sysctl -q proc.cmdline=0400
   # sysctl -q proc.maps=0400
   # sysctl -q proc.stat=0400
   # sysctl -q proc.statm=0400
   # sysctl -q proc.status=0400

This will set the permissions of /proc/*/cmdline etc. to the given
value.

The permissions of files in /proc/1 (usually belonging to init) are
kept as they are.  The idea is to let system processes be freely
visible by anyone, just as before.  Especially interesting in this
regard would be instances of login.  I don't know how to easily
discriminate between system processes and normal processes inside
the kernel (apart from pid == 1 and uid == 0 (which is too broad)).
Any ideas?

It's easy to make more files' permissions configurable, if the need
arises.

This implementation is a lot bigger than the quick hacks I sent earlier.
Is this feature growing too fat?  Also I'm unsure about the #ifdef'ery
in fs/proc/base.c, I just wanted to be consistent with the surrounding
code. :-P

Rene

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH][RFC] /proc umask and gid [was: Make /proc/pid chmod'able]

2005-03-18 Thread Rene Scharfe
On Tue, Mar 15, 2005 at 11:41:28PM -0500, Albert Cahalan wrote:
 Better interface:
 
 /sbin/sysctl -w proc.maps=0440
 /sbin/sysctl -w proc.cmdline=0444
 /sbin/sysctl -w proc.status=0444

Indeed it is, but it's much harder to implement.

Patch is against 2.6.11-mm3 but works with -mm4, too.  I only made the
permissions configurable for some of the files, more can be added easily
if and when needed.

It's uglier than it needs to be, but before cleaning it up I'll go and
get some sleep.

Comments and patches much appreciated! :)

Rene



diff -pur linux-2.6.11-mm3/fs/proc/base.c l5/fs/proc/base.c
--- linux-2.6.11-mm3/fs/proc/base.c 2005-03-12 19:23:36.0 +0100
+++ l5/fs/proc/base.c   2005-03-19 01:08:40.0 +0100
@@ -35,6 +35,7 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/sysctl.h
 #include internal.h
 
 /*
@@ -1419,6 +1420,93 @@ static struct file_operations proc_tgid_
 static struct inode_operations proc_tgid_attr_inode_operations;
 #endif
 
+static struct pid_entry *proc_base_entry_by_name(struct pid_entry *ents,
+ struct qstr *name)
+{
+   struct pid_entry *p;
+
+   for (p = ents; p-name; p++) {
+   if (p-len != name-len)
+   continue;
+   if (!memcmp(name-name, p-name, p-len))
+   return p;
+   }
+   return NULL;
+}
+
+#ifdef CONFIG_SYSCTL
+struct proc_sysctl_info {
+   int type;
+   mode_t mode;
+};
+
+/* order must match the CTL_PROC table in sysctl.h */
+struct proc_sysctl_info proc_base_permissions[] = {
+   {PROC_TGID_CMDLINE, S_IRUGO},
+   {PROC_TGID_MAPS,S_IRUGO},
+   {PROC_TGID_STAT,S_IRUGO},
+   {PROC_TGID_STATM,   S_IRUGO},
+   {PROC_TGID_STATUS,  S_IRUGO},
+   {0, 0}
+};
+
+static inline int is_system_task(struct task_struct *task)
+{
+   int rc = 0;
+
+   if (task-pid == 1)
+   rc = 1;
+   // TODO: kernel threads?
+   return rc;
+}
+
+static void proc_update_mode(mode_t *mode, int type, struct task_struct *task)
+{
+   struct proc_sysctl_info *s;
+
+   if (is_system_task(task))
+   return;
+   for (s = proc_base_permissions; s-type; s++) {
+   if (s-type == type) {
+   *mode = (*mode  ~S_IALLUGO) | s-mode;
+   break;
+   }
+   }
+}
+
+static int proc_base_permission(struct inode *inode, int mask,
+struct nameidata *nd)
+{
+   struct task_struct *task = proc_task(inode);
+   struct pid_entry *p;
+
+   p = proc_base_entry_by_name(tgid_base_stuff, nd-dentry-d_name);
+   if (p)
+   proc_update_mode(inode-i_mode, p-type, task);
+   return generic_permission(inode, mask, NULL);
+}
+
+static int proc_base_getattr(struct vfsmount *mnt, struct dentry *dentry,
+ struct kstat *stat)
+{
+   struct inode *inode = dentry-d_inode;
+   struct task_struct *task = proc_task(inode);
+   struct pid_entry *p;
+
+   generic_fillattr(inode, stat);
+   p = proc_base_entry_by_name(tgid_base_stuff, dentry-d_name);
+   if (p)
+   proc_update_mode(inode-i_mode, p-type, task);
+   return 0;
+}
+
+static struct inode_operations proc_base_inode_operations = {
+   .getattr= proc_base_getattr,
+   .permission = proc_base_permission,
+};
+
+#endif /* CONFIG_SYSCTL */
+
 /* SMP-safe */
 static struct dentry *proc_pident_lookup(struct inode *dir, 
 struct dentry *dentry,
@@ -1436,13 +1524,8 @@ static struct dentry *proc_pident_lookup
if (!pid_alive(task))
goto out;
 
-   for (p = ents; p-name; p++) {
-   if (p-len != dentry-d_name.len)
-   continue;
-   if (!memcmp(dentry-d_name.name, p-name, p-len))
-   break;
-   }
-   if (!p-name)
+   p = proc_base_entry_by_name(ents, dentry-d_name);
+   if (p == NULL)
goto out;
 
error = -EINVAL;
@@ -1452,6 +1535,10 @@ static struct dentry *proc_pident_lookup
 
ei = PROC_I(inode);
inode-i_mode = p-mode;
+#ifdef CONFIG_SYSCTL
+   proc_update_mode(inode-i_mode, p-type, task);
+   inode-i_op = proc_base_inode_operations;
+#endif
/*
 * Yes, it does not scale. And it should not. Don't add
 * new entries into /proc/tgid/ without very good reasons.
diff -pur linux-2.6.11-mm3/include/linux/sysctl.h l5/include/linux/sysctl.h
--- linux-2.6.11-mm3/include/linux/sysctl.h 2005-03-12 19:23:37.0 
+0100
+++ l5/include/linux/sysctl.h   2005-03-18 23:40:59.0 +0100
@@ -654,6 +654,13 @@ enum {
 };
 
 /* CTL_PROC names: */
+enum {
+   PROC_CMDLINE= 1,
+   PROC_MAPS   = 2,
+   PROC_STAT   = 3,
+   PROC_STATM  = 4,
+   

Re: [PATCH][RFC] Make /proc/pid chmod'able

2005-03-15 Thread Rene Scharfe
Albert Cahalan wrote:
Note that the admin hopefully does not normally run as root.
The admin should be using a normal user account most of the
time, to reduce the damage caused by his accidents.
Openwall and GrSecurity solved this by having a special group that can 
see everything, just like root.  E.g. we could add a proc.gid kernel 
boot option for that purpose.

Even if the admin were not running as a normal user, it is
expected that normal users can keep tabs on each other.
The admin may be sleeping. Social pressure is important to
prevent one user from sucking up all the memory and CPU time.
IANAL, but creating a user profile (who ran what when, used how many 
resources etc.) without the user's consent is illegal at least here in 
Germany.  As an admin I'd like to be able to prevent a user from even 
trying to spy on another user.

Anything provided by traditional UNIX and BSD systems
should be available. Users who want privacy can get their
own computer. So, these need to work:
ps -ef
ps -el
ps -ej
ps axu
ps axl
ps axj
ps axv
w
top
If with work you mean show info about all users then the patch 
becomes pointless.  The programs work in the sense that they do *not* 
should cloaked processes, which is intended. :)

OK, I understand that you need to be able to turn this feature off and I 
also don't want non-root admins to suddenly go blind.  Would adding a 
proc.gid kernel parameter and an off-switch be sufficient for you?

Thanks,
Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH][RFC] Make /proc/pid chmod'able

2005-03-15 Thread Rene Scharfe
Albert Cahalan wrote:
This really isn't about security. Privacy may be undesirable.
I agree, privacy is not security.  My patch tries to enhance privacy 
without giving up security.

You think losing the social pressure that comes with mutual surveillance 
results in loss of security, I don't.  Now I think Linux should support 
both ways and those writing security policies should make the decision.

With privacy comes anti-social behavior. Supposing that the
users do get privacy, perhaps because the have paid for it:
Xen, UML, VM, VMware, separate computers
Going with separate computers is best. Don't forget to use
network traffic control to keep users from being able to
detect the network activity of other users.
That would work, but it requires a *lot* of administrative and computing 
overhead.  Note that separate computers alone is not sufficient 
because most places with more than a few machines have some kind of 
single signon and run SSH or similar.

[ps, w, top]
They work like they do with a rootkit installed.
Traditional behavior has been broken.
That's one way to put it; you could also say those tools now provide 
enhanced privacy. ;)

I also think things have changed in the last few years.  Since the 
advent of special data processing laws privacy is taken more serious. 
Privacy certainly was no real concern when UNIX was young.  I also guess 
it's a cultural thing, its importance being different from country to 
country.

It's easily visible in the style of public toilets: in some contries you 
have one big room with no walls in between where all men or women 
merrily shit together, in other countries (like mine) every person can 
lock himself into a private closet.  Both ways work, there's nothing too 
special about using a toilet, but I'm simply used to the privacy 
provided by those thin walls.  I assure you, I don't do anything evil in 
there. :]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH][RFC] /proc umask and gid [was: Make /proc/pid chmod'able]

2005-03-15 Thread Rene Scharfe
So, I gather from the feedback I've got that chmod'able /proc/pid
would be a bit over the top. 8-)  While providing the easiest and most
intuitive user interface for changing the permissions on those
directories, it is overkill.  Paul is right when he says that such a
feature should be turned on or off for all sessions at once, and that's
it.

My patch had at least one other problem: the contents of eac
/proc/pid directory became chmod'able, too, which was not intended.

Instead of fixing it up I took two steps back, dusted off the umask
kernel parameter patch and added the special gid feature I mentioned.

Without the new kernel parameters behaviour is unchanged.  Add
proc.umask=077 and all /proc/pid will get a permission mode of 500.
This breaks pstree (no output), as Bodo already noted, because this
program needs access to /proc/1.  It also breaks w -- it shows the
correct number of users but it lists X even for sessions owned
by the user running it.

Use proc.umask=007 and proc.gid=50 instead and all /proc/pid dirs
will have a mode of 550 and their group attribute will be set to 50
(that's staff on my Debian system).  Pstree will work for all members
of that special group (just like top, ps and w -- which also show
everything in that case).  Normal users will still have a restricted
view.

Albert, would you take fixes for w even though you despise the feature
that makes them necessary?

Is this less scary?  Still useful?

Thanks,
Rene



diff -urp linux-2.6.11-mm3/Documentation/kernel-parameters.txt 
l5/Documentation/kernel-parameters.txt
--- linux-2.6.11-mm3/Documentation/kernel-parameters.txt2005-03-12 
19:23:30.0 +0100
+++ l5/Documentation/kernel-parameters.txt  2005-03-16 01:14:05.0 
+0100
@@ -1095,16 +1095,22 @@ running once the system is up.
[ISAPNP] Exclude memory regions for the 
autoconfiguration
Ranges are in pairs (memory base and size).
 
+   processor.max_cstate=   [HW, ACPI]
+   Limit processor to maximum C-state
+   max_cstate=9 overrides any DMI blacklist limit.
+
+   proc.gid=   [KNL] If non-zero all /proc/pid directories will
+   have their group attribute set to that value.
+
+   proc.umask= [KNL] Restrict permissions of process specific
+   entries in /proc (i.e. the numerical directories).
+
profile=[KNL] Enable kernel profiling via /proc/profile
{ schedule | number }
(param: schedule - profile schedule points}
(param: profile step/bucket size as a power of 2 for
statistical time based profiling)
 
-   processor.max_cstate=   [HW, ACPI]
-   Limit processor to maximum C-state
-   max_cstate=9 overrides any DMI blacklist limit.
-
prompt_ramdisk= [RAM] List of RAM disks to prompt for floppy disk
before loading.
See Documentation/ramdisk.txt.
diff -urp linux-2.6.11-mm3/fs/proc/base.c l5/fs/proc/base.c
--- linux-2.6.11-mm3/fs/proc/base.c 2005-03-12 19:23:36.0 +0100
+++ l5/fs/proc/base.c   2005-03-16 01:54:52.0 +0100
@@ -35,8 +35,18 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/module.h
+#include linux/moduleparam.h
 #include internal.h
 
+static umode_t proc_umask;
+module_param_named(umask, proc_umask, ushort, 0);
+MODULE_PARM_DESC(umask, umask for all /proc/pid entries);
+
+static gid_t proc_gid;
+module_param_named(gid, proc_gid, uint, 0);
+MODULE_PARM_DESC(gid, group attribute of all /proc/pid entries);
+
 /*
  * For hysterical raisins we keep the same inumbers as in the old procfs.
  * Feel free to change the macro below - just keep the range distinct from
@@ -1149,6 +1159,8 @@ static struct inode *proc_pid_make_inode
inode-i_uid = task-euid;
inode-i_gid = task-egid;
}
+   if ((ino == PROC_TGID_INO || ino == PROC_TID_INO)  proc_gid)
+   inode-i_gid = proc_gid;
security_task_to_inode(task, inode);
 
 out:
@@ -1182,6 +1194,8 @@ static int pid_revalidate(struct dentry 
inode-i_uid = 0;
inode-i_gid = 0;
}
+   if ((proc_type(inode) == PROC_TGID_INO || proc_type(inode) == 
PROC_TID_INO)  proc_gid)
+   inode-i_gid = proc_gid;
security_task_to_inode(task, inode);
return 1;
}
@@ -1797,7 +1811,7 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = S_IFDIR | ((S_IRUGO|S_IXUGO)  ~proc_umask);
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;

Re: [PATCH][RFC] Make /proc/pid chmod'able

2005-03-14 Thread Rene Scharfe
Albert Cahalan wrote:
This is a bad idea. Users should not be allowed to
make this decision. This is rightly a decision for
the admin to make.
Why do you think users should not be allowed to chmod their processes' 
/proc directories?  Isn't it similar to being able to chmod their home 
directories?  They own both objects, after all (both conceptually and as 
attributed in the filesystem).

Note: I'm the procps (ps, top, w, etc.) maintainer.
Probably I'd have to make /bin/ps run setuid root
to deal with this. (minor changes needed) The same
goes for /usr/bin/top, which I know is currently
unsafe and difficult to fix.
Let's not go there, OK?
I have to admit to not having done any real testing with those 
utilities.  My excuse is this isn't such a new feature, Openwall had 
something similar for at least four years now and GrSecurity contains 
yet another flavour of it.  Openwall also provides one patch for 
procps-2.0.6, so I figured that problem (whatever their patch is about) 
got fixed in later versions.

Why do ps and top need to be setuid root to deal with a resticted /proc? 
   What information in /proc/pid needs to be available to any and all 
users?

If you restricted this new ability to root, then I'd
have much less of an objection. (not that I'd like it)
How about a boot parameter or sysctl to enable the chmod'ability of 
/proc/pid, defaulting to off?  But I'd like to resolve your more 
general objections above first, if possible. :)

Thanks for your comments,
Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][RFC] Make /proc/pid chmod'able

2005-03-13 Thread Rene Scharfe
OK, folks, another try to enhance privacy by hiding process details
from other users.  Why not simply use chmod to set the permissions of
/proc/pid directories?  This patch implements it.

Children processes inherit their parents' proc permissions on fork.  You
can only set (and remove) read and execute permissions, the bits for
write, suid etc. are not changable.  A user would add

chmod 500 /proc/$$

or something similar to his .profile to cloak his processes.

What do you think about that one?

Thanks,
Rene



diff -urp linux-2.6.11-mm3/fs/proc/base.c linux-2.6.11-mm3+/fs/proc/base.c
--- linux-2.6.11-mm3/fs/proc/base.c 2005-03-12 19:23:36.0 +0100
+++ linux-2.6.11-mm3+/fs/proc/base.c2005-03-13 18:36:06.0 +0100
@@ -1605,6 +1605,55 @@ out:
return ERR_PTR(error);
 }
 
+static int proc_base_setattr(struct dentry *dentry, struct iattr *attr)
+{
+   struct inode *inode = dentry-d_inode;
+   struct task_struct *task;
+   unsigned id;
+   int error;
+
+   BUG_ON(!inode);
+
+   error = -EPERM;
+   if (attr-ia_valid != (ATTR_MODE | ATTR_CTIME))
+   goto out;
+   if (attr-ia_mode  S_IALLUGO  ~(S_IRUGO | S_IXUGO))
+   goto out;
+
+   error = inode_change_ok(inode, attr);
+   if (error)
+   goto out;
+
+   error = -ENOENT;
+   id = name_to_int(dentry);
+   if (id == ~0U)
+   goto out;
+
+   read_lock(tasklist_lock);
+   task = find_task_by_pid(id);
+   if (task)
+   get_task_struct(task);
+   read_unlock(tasklist_lock);
+   if (!task)
+   goto out;
+
+   error = inode_setattr(inode, attr);
+   if (error)
+   goto out_drop_task;
+   /*
+* Save permissions in task_struct as the reverse of the mode.
+* This way a value of zero, which is the default value of a
+* task_struct member, means normal permissions.  Children
+* inherit the proc_dir_mask value of their parent process.
+*/
+   task-proc_dir_mask = S_IRWXUGO - (attr-ia_mode  S_IRWXUGO);
+
+out_drop_task:
+   put_task_struct(task);
+out:
+   return error;
+}
+
 static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry 
*dentry, struct nameidata *nd){
return proc_pident_lookup(dir, dentry, tgid_base_stuff);
 }
@@ -1625,10 +1674,12 @@ static struct file_operations proc_tid_b
 
 static struct inode_operations proc_tgid_base_inode_operations = {
.lookup = proc_tgid_base_lookup,
+   .setattr= proc_base_setattr,
 };
 
 static struct inode_operations proc_tid_base_inode_operations = {
.lookup = proc_tid_base_lookup,
+   .setattr= proc_base_setattr,
 };
 
 #ifdef CONFIG_SECURITY
@@ -1797,11 +1848,10 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~task-proc_dir_mask;
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;
inode-i_nlink = 3;
-   inode-i_flags|=S_IMMUTABLE;
 
dentry-d_op = pid_base_dentry_operations;
 
@@ -1852,11 +1902,10 @@ static struct dentry *proc_task_lookup(s
 
if (!inode)
goto out_drop_task;
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~task-proc_dir_mask;
inode-i_op = proc_tid_base_inode_operations;
inode-i_fop = proc_tid_base_operations;
inode-i_nlink = 3;
-   inode-i_flags|=S_IMMUTABLE;
 
dentry-d_op = pid_base_dentry_operations;
 
diff -urp linux-2.6.11-mm3/include/linux/sched.h 
linux-2.6.11-mm3+/include/linux/sched.h
--- linux-2.6.11-mm3/include/linux/sched.h  2005-03-12 19:23:37.0 
+0100
+++ linux-2.6.11-mm3+/include/linux/sched.h 2005-03-13 11:54:13.0 
+0100
@@ -719,6 +719,10 @@ struct task_struct {
struct audit_context *audit_context;
seccomp_t seccomp;
 
+#ifdef CONFIG_PROC_FS
+   umode_t proc_dir_mask;
+#endif
+
 /* Thread group tracking */
u32 parent_exec_id;
u32 self_exec_id;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH][RFC] Apply umask to /proc/pid

2005-03-12 Thread Rene Scharfe
Hi all,

patch below makes procfs apply the umask of the processes to their
respective /proc/pid directories and the files below them.  That
allows users to gain a bit of privacy by setting a restrictive umask in
their profiles.

Unlike my previous patches on that subject it requires no new parameter
or mount option and puts control into the hands of the user owning those
files.  And it's even smaller and simpler, too. :]

A umask change only affects processes created after the change with
certainty.  The /proc files of a process changing its own umask may or
may not be affected, depending on whether their inode already exists.
That's OK, I think, and fits within the conventional meaning of umask.

Tools like top and ps continue to work, they simply don't show processes
that the user running them has no permission to get details on.  Even
pstree works as long as the umask of init is kept at its permissive default
value.

Comments welcome!

Rene



--- linux-2.6.11-bk8/fs/proc/base.c~2005-03-12 19:11:37.0 +0100
+++ linux-2.6.11-bk8/fs/proc/base.c 2005-03-12 19:12:53.0 +0100
@@ -1418,6 +1418,8 @@ static struct file_operations proc_tgid_
 static struct inode_operations proc_tgid_attr_inode_operations;
 #endif
 
+#define proc_get_umask(task) (task-fs ? task-fs-umask : 0)
+
 /* SMP-safe */
 static struct dentry *proc_pident_lookup(struct inode *dir, 
 struct dentry *dentry,
@@ -1450,7 +1452,7 @@ static struct dentry *proc_pident_lookup
goto out;
 
ei = PROC_I(inode);
-   inode-i_mode = p-mode;
+   inode-i_mode = p-mode  ~proc_get_umask(task);
/*
 * Yes, it does not scale. And it should not. Don't add
 * new entries into /proc/tgid/ without very good reasons.
@@ -1796,7 +1798,7 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~proc_get_umask(task);
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;
inode-i_nlink = 3;
@@ -1851,7 +1853,7 @@ static struct dentry *proc_task_lookup(s
 
if (!inode)
goto out_drop_task;
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~proc_get_umask(task);
inode-i_op = proc_tid_base_inode_operations;
inode-i_fop = proc_tid_base_operations;
inode-i_nlink = 3;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] add umask parameter to procfs

2005-02-18 Thread Rene Scharfe
On Fri, Feb 18, 2005 at 04:56:01AM +0100, Herbert Poetzl wrote:
 hmm, so what about debugger and similar not able to find the
 parent process or something like that?

You can walk the parentage chain up until you reach your login shell.
So you can look up info about the parent of every one of your processes
except for your login shell and any zombies.

 I'd say this needs some more investigation what tools and
 applications will break once it is enabled ...

Sure, that can't be bad.  I didn't really do anything so far that
warrants being called testing (compiles, runs, doesn't crash on boot --
send patch! ;).

But I also didn't invent this feature: it has been in OpenWall and
grsecurity for a long time now, so a form of restricted /proc has
received at least *some* testing.

Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] remove mount option parsing from procfs

2005-02-17 Thread Rene Scharfe
OK, my previous patches on the subject were _so_ bad that noone even
bothered to flame me.  Here's an attempt to fix this. :]

This patch removes the mount options of the proc filesystem.  They don't
have any effect since 2.4.something.

Explanation: Only proc_fill_super() calls parse_options, notably
proc_remount() does not.  And proc_fill_super() is only called at the
very first mount which in turn is the one caused by kern_mount() in
fs/proc/root.c and that passes a NULL pointer as mount options
string.  It is called only once because proc is a filesystem with a
single super_block (i.e. it uses get_sb_single()).

Since noone seems to miss the uid and gid options so far I suggest to
simply remove them.  Their function can be easily performed in
userspace.  E.g. this (if it worked like intended):

# mount -t proc -o uid=procuser,gid=procgrp proc /proc

can be done like so, probably in some init script:

# mount -t proc proc /proc  chown procuser:procgrp /proc

But I don't see why anyone would want to do that in the first place.

Comments?

Rene


--- linux-2.6.11-rc4/fs/proc/inode.c~   2005-02-05 03:21:58.0 +0100
+++ linux-2.6.11-rc4/fs/proc/inode.c2005-02-11 05:33:47.0 +0100
@@ -14,7 +14,6 @@
 #include linux/limits.h
 #include linux/init.h
 #include linux/module.h
-#include linux/parser.h
 #include linux/smp_lock.h
 
 #include asm/system.h
@@ -143,51 +142,6 @@ static struct super_operations proc_sops
.remount_fs = proc_remount,
 };
 
-enum {
-   Opt_uid, Opt_gid, Opt_err
-};
-
-static match_table_t tokens = {
-   {Opt_uid, uid=%u},
-   {Opt_gid, gid=%u},
-   {Opt_err, NULL}
-};
-
-static int parse_options(char *options,uid_t *uid,gid_t *gid)
-{
-   char *p;
-   int option;
-
-   *uid = current-uid;
-   *gid = current-gid;
-   if (!options)
-   return 1;
-
-   while ((p = strsep(options, ,)) != NULL) {
-   substring_t args[MAX_OPT_ARGS];
-   int token;
-   if (!*p)
-   continue;
-
-   token = match_token(p, tokens, args);
-   switch (token) {
-   case Opt_uid:
-   if (match_int(args, option))
-   return 0;
-   *uid = option;
-   break;
-   case Opt_gid:
-   if (match_int(args, option))
-   return 0;
-   *gid = option;
-   break;
-   default:
-   return 0;
-   }
-   }
-   return 1;
-}
-
 struct inode *proc_get_inode(struct super_block *sb, unsigned int ino,
struct proc_dir_entry *de)
 {
@@ -249,10 +203,11 @@ int proc_fill_super(struct super_block *
 * Fixup the root inode's nlink value
 */
root_inode-i_nlink += nr_processes();
+   root_inode-i_uid = 0;
+   root_inode-i_gid = 0;
s-s_root = d_alloc_root(root_inode);
if (!s-s_root)
goto out_no_root;
-   parse_options(data, root_inode-i_uid, root_inode-i_gid);
return 0;
 
 out_no_root:
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] add umask parameter to procfs

2005-02-17 Thread Rene Scharfe
Add proc.umask kernel parameter.  It can be used to restrict permissions
on the numerical directories in the root of a proc filesystem, i.e. the
directories containing process specific information.

E.g. add proc.umask=077 to your kernel command line and all users except
root can only see their own process details (like command line
parameters) with ps or top.  It can be useful to add a bit of privacy to
multi-user servers.

The patch has been inspired by a similar feature in GrSecurity.

It could have also been implemented as a mount option to procfs, but at
a higher cost and no apparent benefit -- changes to this umask are not
supposed to happen very often.  Actually, the previous incarnation of
this patch was implemented as a half-assed mount option, but I didn't
know then how easy it is to add a kernel parameter.

Comments about usefulness or implementation are very welcome.

Thanks,
Rene


--- linux-2.6.11-rc4/fs/proc/base.c~2005-02-11 21:16:13.0 +0100
+++ linux-2.6.11-rc4/fs/proc/base.c 2005-02-11 23:38:17.0 +0100
@@ -32,8 +32,14 @@
 #include linux/mount.h
 #include linux/security.h
 #include linux/ptrace.h
+#include linux/module.h
+#include linux/moduleparam.h
 #include internal.h
 
+static umode_t umask = 0;
+module_param(umask, ushort, 0);
+MODULE_PARM_DESC(umask, umask for the numerical directories in /proc);
+
 /*
  * For hysterical raisins we keep the same inumbers as in the old procfs.
  * Feel free to change the macro below - just keep the range distinct from
@@ -1369,7 +1375,7 @@ static struct dentry *proc_pident_lookup
goto out;
 
ei = PROC_I(inode);
-   inode-i_mode = p-mode;
+   inode-i_mode = p-mode  ~(umask  S_IRWXUGO);
/*
 * Yes, it does not scale. And it should not. Don't add
 * new entries into /proc/tgid/ without very good reasons.
@@ -1699,7 +1705,7 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = S_IFDIR | ((S_IRUGO|S_IXUGO)  ~umask);
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;
inode-i_nlink = 3;
@@ -1754,7 +1760,7 @@ static struct dentry *proc_task_lookup(s
 
if (!inode)
goto out_drop_task;
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = S_IFDIR | ((S_IRUGO|S_IXUGO)  ~umask);
inode-i_op = proc_tid_base_inode_operations;
inode-i_fop = proc_tid_base_operations;
inode-i_nlink = 3;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] proc filesystem (1/3): fix mount options

2005-02-01 Thread rene . scharfe
Hello,

while trying to add a umask option to the proc filesystem I stumbled
over a somewhat related problem: the existing mount options uid and
gid were non-functional.  The patch below is an attempt to fix them
and prepares the ground for my evil umask plans. :)

The first half of the reason why the uid and gid options are not
working is that proc has only a single superblock, no matter how many
times it is mounted.  This is achieved with the help of get_single_sb().
Only the first mount calls fill_super(), all following mounts are
like remounts.

The second half is that proc is mounted during boot with kern_mount().
This function passes NULL as mount parameter string to
proc_fill_super().

So the single mount call where we could provide some options provides --
nothing.  I don't know why parse_options() is not called from
proc_remount(), but it looks like this has been done on purpose, to
avoid changing ownership of the root inode while proc is mounted.
Is that observation correct or was it just an accident?

Anyway, the following patch changes proc to not appear like accepting
regular mount options and instead makes it parse kernel parameters.  I
hope this is not too evil.  They provide the semantics which I hope is
correct: changable at boot time and only at boot time.

Patch is against 2.6.11-rc2-bk10 (2.6.10 should be OK, too), compiles,
boots and works for me.

Comments welcome.

Thanks,
Rene


diff -pur linux-2.6.11-rc2-bk10/fs/proc/inode.c ll/fs/proc/inode.c
--- linux-2.6.11-rc2-bk10/fs/proc/inode.c   2005-02-01 04:17:25.0 
+0100
+++ ll/fs/proc/inode.c  2005-02-01 03:23:51.0 +0100
@@ -148,22 +148,24 @@ enum {
 };
 
 static match_table_t tokens = {
-   {Opt_uid, uid=%u},
-   {Opt_gid, gid=%u},
+   {Opt_uid, proc.uid=%u},
+   {Opt_gid, proc.gid=%u},
{Opt_err, NULL}
 };
 
+/*
+ * This parse_options function is different.  It parses kernel parameters
+ * instead of filesystem mount options.
+ */ 
 static int parse_options(char *options,uid_t *uid,gid_t *gid)
 {
char *p;
int option;
 
-   *uid = current-uid;
-   *gid = current-gid;
if (!options)
return 1;
 
-   while ((p = strsep(options, ,)) != NULL) {
+   while ((p = strsep(options,  \t)) != NULL) {
substring_t args[MAX_OPT_ARGS];
int token;
if (!*p)
@@ -173,16 +175,14 @@ static int parse_options(char *options,u
switch (token) {
case Opt_uid:
if (match_int(args, option))
-   return 0;
+   continue;
*uid = option;
break;
case Opt_gid:
if (match_int(args, option))
-   return 0;
+   continue;
*gid = option;
break;
-   default:
-   return 0;
}
}
return 1;
@@ -231,6 +231,11 @@ out_fail:
goto out;
 }  
 
+/*
+ * Because proc is a single-superblock filesystem, proc_fill_super() is
+ * only called at boot time and never during sys_mount().  That means
+ * filesystem options can only be specified as kernel parameters.
+ */
 int proc_fill_super(struct super_block *s, void *data, int silent)
 {
struct inode * root_inode;
@@ -249,6 +254,8 @@ int proc_fill_super(struct super_block *
 * Fixup the root inode's nlink value
 */
root_inode-i_nlink += nr_processes();
+   root_inode-i_uid = 0;
+   root_inode-i_gid = 0;
s-s_root = d_alloc_root(root_inode);
if (!s-s_root)
goto out_no_root;
diff -pur linux-2.6.11-rc2-bk10/fs/proc/root.c ll/fs/proc/root.c
--- linux-2.6.11-rc2-bk10/fs/proc/root.c2004-12-24 22:35:24.0 
+0100
+++ ll/fs/proc/root.c   2005-02-01 02:25:04.0 +0100
@@ -7,6 +7,7 @@
  */
 
 #include asm/uaccess.h
+#include asm/setup.h
 
 #include linux/errno.h
 #include linux/time.h
@@ -17,6 +18,8 @@
 #include linux/module.h
 #include linux/bitops.h
 #include linux/smp_lock.h
+#include linux/string.h
+#include linux/mount.h
 
 struct proc_dir_entry *proc_net, *proc_net_stat, *proc_bus, *proc_root_fs, 
*proc_root_driver;
 
@@ -39,13 +42,15 @@ static struct file_system_type proc_fs_t
 extern int __init proc_init_inodecache(void);
 void __init proc_root_init(void)
 {
+   char tmp_cmdline[COMMAND_LINE_SIZE];
int err = proc_init_inodecache();
if (err)
return;
err = register_filesystem(proc_fs_type);
if (err)
return;
-   proc_mnt = kern_mount(proc_fs_type);
+   strlcpy(tmp_cmdline, saved_command_line, COMMAND_LINE_SIZE);
+   proc_mnt = do_kern_mount(proc, 0, proc, tmp_cmdline);
err = PTR_ERR(proc_mnt);
if (IS_ERR(proc_mnt)) {

[PATCH] proc filesystem (3/3): add proc_show_options()

2005-02-01 Thread rene . scharfe
This patch adds a show_options function to the proc filesystem.

diff -pur l2/fs/proc/inode.c l3/fs/proc/inode.c
--- l2/fs/proc/inode.c  2005-02-01 04:51:23.0 +0100
+++ l3/fs/proc/inode.c  2005-02-01 04:51:07.0 +0100
@@ -16,6 +16,7 @@
 #include linux/module.h
 #include linux/parser.h
 #include linux/smp_lock.h
+#include linux/seq_file.h
 
 #include asm/system.h
 #include asm/uaccess.h
@@ -23,6 +24,9 @@
 extern umode_t proc_umask;
 extern void free_proc_entry(struct proc_dir_entry *);
 
+static int proc_root_inode_uid;
+static int proc_root_inode_gid;
+
 static inline struct proc_dir_entry * de_get(struct proc_dir_entry *de)
 {
if (de)
@@ -134,6 +138,17 @@ static int proc_remount(struct super_blo
return 0;
 }
 
+static int proc_show_options(struct seq_file *m, struct vfsmount *mnt)
+{
+   if (proc_root_inode_uid != 0)
+   seq_printf(m, ,uid=%i, proc_root_inode_uid);
+   if (proc_root_inode_gid != 0)
+   seq_printf(m, ,gid=%i, proc_root_inode_gid);
+   if (proc_umask != 0)
+   seq_printf(m, ,umask=%04o, proc_umask);
+   return 0;
+}
+
 static struct super_operations proc_sops = { 
.alloc_inode= proc_alloc_inode,
.destroy_inode  = proc_destroy_inode,
@@ -142,6 +157,7 @@ static struct super_operations proc_sops
.delete_inode   = proc_delete_inode,
.statfs = simple_statfs,
.remount_fs = proc_remount,
+   .show_options   = proc_show_options,
 };
 
 enum {
@@ -248,6 +264,8 @@ int proc_fill_super(struct super_block *
struct inode * root_inode;
 
proc_umask = 0;
+   proc_root_inode_uid = 0;
+   proc_root_inode_gid = 0;
s-s_flags |= MS_NODIRATIME;
s-s_blocksize = 1024;
s-s_blocksize_bits = 10;
@@ -262,12 +280,12 @@ int proc_fill_super(struct super_block *
 * Fixup the root inode's nlink value
 */
root_inode-i_nlink += nr_processes();
-   root_inode-i_uid = 0;
-   root_inode-i_gid = 0;
s-s_root = d_alloc_root(root_inode);
if (!s-s_root)
goto out_no_root;
-   parse_options(data, root_inode-i_uid, root_inode-i_gid);
+   parse_options(data, proc_root_inode_uid, proc_root_inode_gid);
+   root_inode-i_uid = proc_root_inode_uid;
+   root_inode-i_gid = proc_root_inode_gid;
return 0;
 
 out_no_root:
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] proc filesystem (2/3): add umask option

2005-02-01 Thread rene . scharfe
This patch adds the umask option to the proc filesystem.  It's
essentially unchanged from the previous version, except that the mount
option has to be specified as a kernel parameter now.  This way we avoid
dealing with pre-existing inodes.

The umask can be used to restrict the permissions of process information
in /proc (the numerical directories and the files within them).  No other
file or directory should be affected.

Al, there was a big IF in your comment regarding the previous version.  
Do you have general objections against the umask feature or its
usefulness?

Thanks,
Rene


diff -pur l1/fs/proc/base.c l2/fs/proc/base.c
--- l1/fs/proc/base.c   2005-02-01 04:17:25.0 +0100
+++ l2/fs/proc/base.c   2005-02-01 04:26:03.0 +0100
@@ -180,6 +180,8 @@ static struct pid_entry tid_attr_stuff[]
 
 #undef E
 
+umode_t proc_umask;
+
 static int proc_fd_link(struct inode *inode, struct dentry **dentry, struct 
vfsmount **mnt)
 {
struct task_struct *task = proc_task(inode);
@@ -1222,7 +1224,7 @@ static struct dentry *proc_pident_lookup
goto out;
 
ei = PROC_I(inode);
-   inode-i_mode = p-mode;
+   inode-i_mode = p-mode  ~proc_umask;
/*
 * Yes, it does not scale. And it should not. Don't add
 * new entries into /proc/tgid/ without very good reasons.
@@ -1537,7 +1539,7 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~proc_umask;
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;
inode-i_nlink = 3;
@@ -1592,7 +1594,7 @@ static struct dentry *proc_task_lookup(s
 
if (!inode)
goto out_drop_task;
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~proc_umask;
inode-i_op = proc_tid_base_inode_operations;
inode-i_fop = proc_tid_base_operations;
inode-i_nlink = 3;
diff -pur l1/fs/proc/inode.c l2/fs/proc/inode.c
--- l1/fs/proc/inode.c  2005-02-01 04:29:33.0 +0100
+++ l2/fs/proc/inode.c  2005-02-01 04:24:08.0 +0100
@@ -20,6 +20,7 @@
 #include asm/system.h
 #include asm/uaccess.h
 
+extern umode_t proc_umask;
 extern void free_proc_entry(struct proc_dir_entry *);
 
 static inline struct proc_dir_entry * de_get(struct proc_dir_entry *de)
@@ -144,12 +145,13 @@ static struct super_operations proc_sops
 };
 
 enum {
-   Opt_uid, Opt_gid, Opt_err
+   Opt_uid, Opt_gid, Opt_umask, Opt_err
 };
 
 static match_table_t tokens = {
{Opt_uid, proc.uid=%u},
{Opt_gid, proc.gid=%u},
+   {Opt_umask, proc.umask=%o},
{Opt_err, NULL}
 };
 
@@ -183,6 +185,11 @@ static int parse_options(char *options,u
continue;
*gid = option;
break;
+   case Opt_umask:
+   if (match_octal(args, option))
+   continue;
+   proc_umask = option  0777;
+   break;
}
}
return 1;
@@ -240,6 +247,7 @@ int proc_fill_super(struct super_block *
 {
struct inode * root_inode;
 
+   proc_umask = 0;
s-s_flags |= MS_NODIRATIME;
s-s_blocksize = 1024;
s-s_blocksize_bits = 10;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] Restrict procfs permissions

2005-01-29 Thread Rene Scharfe
Al Viro wrote:
On Sat, Jan 29, 2005 at 03:45:42AM +0100, Rene Scharfe wrote:
The patch is inspired by the /proc restriction parts of the
GrSecurity patch.  The main difference is the ability to configure
the restrictions dynamically.  You can change the umask setting by
running
# mount -o remount,umask=007 /proc
Testing has been *very* light so far -- it compiles and boots.
Patch is against 2.6.11-rc2-bk6.
Comments are very welcome.

It leaves already existing inodes with whatever mode they used to
have.
I said configure the restrictions dynamically but I meant doesn't
need a recompile to change settings.  I expect the umask to be
specified in /etc/fstab and rarely changed in a running system.  With
that in mind I think the patch is useful as-is, especially because it's
so small.  But I agree, that thing is a dirty hack. :]
_IF_ you want to do that sort of things, do it right - add
-permission() that would apply that umask before checks and if you
want it to be seen in results of stat(2) - add -gettattr() and do
the same there.
Aww, that sounds expensive.  My favourite solution would be to only 
allow the umask to be changed at mount time, not when remounting.

Calling parse_options from proc_fill_super, only and not from 
proc_remount does not help very much because proc_fill_super is only 
called at boot (or proc module load time).  Is there another way?

While we are here: how would one change the uid or gid parameter? With a 
built-in proc fs the mount -a -t proc in the init scripts only results 
in a proc_remount call which (in mainline) doesn't bother looking at 
parameters at all.  The same is true for a unmount, mount sequence.

Thanks,
Rene
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] Restrict procfs permissions

2005-01-28 Thread Rene Scharfe
Hi all,

this patch adds a umask option to the proc filesystem.  It can be used
to restrict the permission of users to view each others process
information.  E.g. on a multi-user shell server one could use a setting
of umask=077 to allow all users to view info about their own processes,
only.  It should prevent command line snooping and generally increases
privacy on the server.

Top and ps can cope with such restrictions, they simply are quiet about
files they cannot access.

The umask option affects permissions of the numerical directories in
/proc, only (the process info).  And root can see everything, of course,
even with a umask setting of 0777.  Default umask is 0, i.e. unchanged
permissions.

The patch is inspired by the /proc restriction parts of the GrSecurity
patch.  The main difference is the ability to configure the restrictions
dynamically.  You can change the umask setting by running

   # mount -o remount,umask=007 /proc

Testing has been *very* light so far -- it compiles and boots.  Patch is
against 2.6.11-rc2-bk6.

Comments are very welcome.

Thanks,
Rene


diff -rup linux-2.6.11-rc2-bk6/fs/proc/base.c l/fs/proc/base.c
--- linux-2.6.11-rc2-bk6/fs/proc/base.c 2005-01-28 23:42:44.0 +
+++ l/fs/proc/base.c2005-01-28 23:58:38.0 +
@@ -1222,7 +1222,7 @@ static struct dentry *proc_pident_lookup
goto out;
 
ei = PROC_I(inode);
-   inode-i_mode = p-mode;
+   inode-i_mode = p-mode  ~proc_umask;
/*
 * Yes, it does not scale. And it should not. Don't add
 * new entries into /proc/tgid/ without very good reasons.
@@ -1537,7 +1537,7 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~proc_umask;
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;
inode-i_nlink = 3;
@@ -1592,7 +1592,7 @@ static struct dentry *proc_task_lookup(s
 
if (!inode)
goto out_drop_task;
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = (S_IFDIR|S_IRUGO|S_IXUGO)  ~proc_umask;
inode-i_op = proc_tid_base_inode_operations;
inode-i_fop = proc_tid_base_operations;
inode-i_nlink = 3;
diff -rup linux-2.6.11-rc2-bk6/fs/proc/inode.c l/fs/proc/inode.c
--- linux-2.6.11-rc2-bk6/fs/proc/inode.c2005-01-28 23:42:44.0 
+
+++ l/fs/proc/inode.c   2005-01-28 23:56:11.0 +
@@ -22,6 +22,8 @@
 
 extern void free_proc_entry(struct proc_dir_entry *);
 
+umode_t proc_umask = 0;
+
 static inline struct proc_dir_entry * de_get(struct proc_dir_entry *de)
 {
if (de)
@@ -127,9 +129,14 @@ int __init proc_init_inodecache(void)
return 0;
 }
 
+static int parse_options(char *, uid_t *, gid_t *);
 static int proc_remount(struct super_block *sb, int *flags, char *data)
 {
+   uid_t dummy_uid;
+   gid_t dummy_gid;
+
*flags |= MS_NODIRATIME;
+   parse_options(data, dummy_uid, dummy_gid);
return 0;
 }
 
@@ -144,12 +151,13 @@ static struct super_operations proc_sops
 };
 
 enum {
-   Opt_uid, Opt_gid, Opt_err
+   Opt_uid, Opt_gid, Opt_umask, Opt_err
 };
 
 static match_table_t tokens = {
{Opt_uid, uid=%u},
{Opt_gid, gid=%u},
+   {Opt_umask, umask=%o},
{Opt_err, NULL}
 };
 
@@ -181,6 +189,11 @@ static int parse_options(char *options,u
return 0;
*gid = option;
break;
+   case Opt_umask:
+   if (match_octal(args, option))
+   return 0;
+   proc_umask = option;
+   break;
default:
return 0;
}
diff -rup linux-2.6.11-rc2-bk6/fs/proc/internal.h l/fs/proc/internal.h
--- linux-2.6.11-rc2-bk6/fs/proc/internal.h 2005-01-28 23:42:44.0 
+
+++ l/fs/proc/internal.h2005-01-28 23:58:29.0 +
@@ -16,6 +16,8 @@ struct vmalloc_info {
unsigned long   largest_chunk;
 };
 
+extern umode_t proc_umask;
+
 #ifdef CONFIG_MMU
 #define VMALLOC_TOTAL (VMALLOC_END - VMALLOC_START)
 extern void get_vmalloc_info(struct vmalloc_info *vmi);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] strtok - strsep (The Easy Cases)

2001-05-04 Thread Rene Scharfe

Am Freitag,  4. Mai 2001 02:57 schrieb Rusty Russell:
 In message 01050120580701.01713@golmepha you write:
  Hello,

Hi!

 
  the patch at the bottom does the bulk job of strtok replacement. It's a
  very boring patch, containing easy cases, only. It became a bit big, too,
  but I trust you can digest it nevertheless. It's made against kernel
  version 2.4.4.

 There are two cases where the substitution is problematic:

Yes, but...

The cases which my patch modifies are of a different kind:

int parse_options(char *options)
{
char *p;

/* for (p = strtok(options, ,); p; p = strtok(NULL, ,)) { */
while (p = strsep(options, ,)) {
/* ... */
}
return 0;
}

No temporary array, no kfree(). Our variable options is used for strtsep,
only. Notice btw. how we destoy the string content to which options
points with both strtok and strsep.

That said, it's possible I made a stupid mistake, of course. Or two. Do
you agree on the correctness of the code above? If not, forget the whole
thing and I'll try again later.



 Array:
   char tmparray[500];
   strcpy(tmparray, str);

   /* for (p = strtok(tmparray, n); p; p = strtok(NULL, n)) { */
   while ((p = strsep(tmparray, ,))) {

 This is clearly wrong, and invokes a compiler warning.  tmparray ==
 tmparray (a cute C oddity I've never really liked).  You are blowing
 away the first few characters in tmparray, and your parser won't work
 properly.

 Dynamic:

   char *tmp = strdup(str);

   /* for (p = strtok(tmp, n); p; p = strtok(NULL, n)) { */
   while ((p = strsep(tmp, ,))) {
   ...
   }

   kfree(tmp);

 Here, tmp has changed in the strsep implementation, and kfree will do
 bad things.

 There is a real reason to avoid strtok, and that is SMP and multple
 threads calling it at once (that said, I don't know of a problem yet).
 But this patch is a step backwards.

 Rusty.
 --
 Premature optmztion is rt of all evl. --DK


René

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



[PATCH] strtok - strsep (The Easy Cases)

2001-05-01 Thread Rene Scharfe

Hello,

the patch at the bottom does the bulk job of strtok replacement. It's a
very boring patch, containing easy cases, only. It became a bit big, too,
but I trust you can digest it nevertheless. It's made against kernel
version 2.4.4.

What is the benefit of getting rid of strtok? It is for cutting strings
into pieces and it's used in argument parsing code of most file
systems and framebuffers. The problem is: strtok is not reentrant and its
manpage tells you to stop using it. There is a replacement function:
strsep. strsep has the added benefit of returning empty tokens, too. We
don't need strtok, it's a bug - do you need any more reasons for replacing
it? In the longer run I want the kernel to be completely cleaned from
strtok - expect more patches to come.

Please apply this patch to the official version of the kernel.


René




diff -ur linux-2.4.4/arch/m68k/atari/config.c linux-2.4.4-rs/arch/m68k/atari/config.c
--- linux-2.4.4/arch/m68k/atari/config.cTue Nov 28 02:57:34 2000
+++ linux-2.4.4-rs/arch/m68k/atari/config.c Tue May  1 17:03:46 2001
@@ -206,13 +206,15 @@
 char *p;
 int ovsc_shift;
 
-/* copy string to local array, strtok works destructively... */
+/* copy string to local array, strsep works destructively... */
 strncpy( switches, str, len );
 switches[len] = 0;
 atari_switches = 0;
 
 /* parse the options */
-for( p = strtok( switches, , ); p; p = strtok( NULL, , ) ) {
+while( p = strsep( switches, , ) ) {
+   if (!*p)
+   continue;
ovsc_shift = 0;
if (strncmp( p, ov_, 3 ) == 0) {
p += 3;
diff -ur linux-2.4.4/drivers/scsi/ibmmca.c linux-2.4.4-rs/drivers/scsi/ibmmca.c
--- linux-2.4.4/drivers/scsi/ibmmca.c   Sat Mar  3 03:38:38 2001
+++ linux-2.4.4-rs/drivers/scsi/ibmmca.cTue May  1 17:48:14 2001
@@ -1406,9 +1406,9 @@
io_base = 0;
id_base = 0;
if (str) {
-  token = strtok(str,,);
   j = 0;
-  while (token) {
+  while (token = strsep(str,,)) {
+if (!*token) continue;
 if (!strcmp(token,activity)) display_mode |= LED_ACTIVITY;
 if (!strcmp(token,display)) display_mode |= LED_DISP;
 if (!strcmp(token,adisplay)) display_mode |= LED_ADISP;
@@ -1424,7 +1424,6 @@
  scsi_id[id_base++] = simple_strtoul(token,NULL,0);
j++;
 }
-token = strtok(NULL,,);
   }
} else if (ints) {
   for (i = 0; i  IM_MAX_HOSTS  2*i+2  ints[0]; i++) {
diff -ur linux-2.4.4/drivers/video/acornfb.c linux-2.4.4-rs/drivers/video/acornfb.c
--- linux-2.4.4/drivers/video/acornfb.c Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/acornfb.c  Tue May  1 17:03:46 2001
@@ -1527,7 +1527,7 @@
 
acornfb_init_fbinfo();
 
-   for (opt = strtok(options, ,); opt; opt = strtok(NULL, ,)) {
+   while (opt = strsep(options, ,)) {
if (!*opt)
continue;
 
diff -ur linux-2.4.4/drivers/video/amifb.c linux-2.4.4-rs/drivers/video/amifb.c
--- linux-2.4.4/drivers/video/amifb.c   Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/amifb.cTue May  1 17:03:46 2001
@@ -1195,7 +1195,9 @@
if (!options || !*options)
return 0;
 
-   for (this_opt = strtok(options, ,); this_opt; this_opt = strtok(NULL, ,)) {
+   while (this_opt = strsep(options, ,)) {
+   if (!*this_opt)
+   continue;
if (!strcmp(this_opt, inverse)) {
amifb_inverse = 1;
fb_invert_cmaps();
diff -ur linux-2.4.4/drivers/video/atafb.c linux-2.4.4-rs/drivers/video/atafb.c
--- linux-2.4.4/drivers/video/atafb.c   Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/atafb.cTue May  1 17:03:46 2001
@@ -2899,7 +2899,7 @@
 if (!options || !*options)
return 0;
  
-for(this_opt=strtok(options,,); this_opt; this_opt=strtok(NULL,,)) {
+while (this_opt = strsep(options, ,)) {
if (!*this_opt) continue;
if ((temp=get_video_mode(this_opt)))
default_par=temp;
diff -ur linux-2.4.4/drivers/video/aty128fb.c linux-2.4.4-rs/drivers/video/aty128fb.c
--- linux-2.4.4/drivers/video/aty128fb.cFri Feb  9 20:30:23 2001
+++ linux-2.4.4-rs/drivers/video/aty128fb.c Tue May  1 17:03:46 2001
@@ -1614,8 +1614,9 @@
 if (!options || !*options)
return 0;
 
-for (this_opt = strtok(options, ,); this_opt;
-this_opt = strtok(NULL, ,)) {
+while (this_opt = strsep(options, ,)) {
+   if (!*this_opt)
+   continue;
if (!strncmp(this_opt, font:, 5)) {
char *p;
int i;
diff -ur linux-2.4.4/drivers/video/atyfb.c linux-2.4.4-rs/drivers/video/atyfb.c
--- linux-2.4.4/drivers/video/atyfb.c   Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/atyfb.cTue May  1 17:03:46 2001
@@ -4062,8 +4062,9 @@
 if (!options || !*options)
return 0;
 
-for 

[PATCH] change strsep() behaviour

2001-04-20 Thread Rene Scharfe

Hello,

some time ago Ingo Oeser tried to replace strtok() and noone seemed to
notice. This time it's me who believes to be on this quest.

Why? strtok() is not reentrant, it uses a global variable to store some
kind of state. As its manpage states: "Don't use this function". But right
now almost every file system and framebuffer uses strtok() for parameter
parsing.

strsep() was created as an replacement. Unfortunatly the implementation
which made it into the kernel misses an important feature: it does not
return empty tokens.

OK, and after applying this patch you'll have a rectified strsep(). It
works against 2.4.3 and -ac10, and should also do so against -pre5. If
this gets accepted, I'm willing to start a crusade against strtok(),
sending patches to the maintainers and all.

There's a minor issue with smbfs which uses the old-style semantics of
strsep(). My patch takes care of this. I could not find any other current
use of strsep() exept in some #if 0'ed piece of code in the Power PC tree.

Any comments? Am I doing something stupid?
Please, cc: me ([EMAIL PROTECTED]) in replies, because I'm not on lkml (yet).

Best regards,
Ren Scharfe


--- linux-2.4.3/lib/string.cWed Apr  4 20:06:39 2001
+++ linux-2.4.3-ac10-rs/lib/string.cFri Apr 20 18:08:56 2001
@@ -326,21 +326,24 @@
  * @ct: The characters to search for
  *
  * strsep() updates @s to point after the token, ready for the next call.
+ *
+ * It returns empty tokens, too, behaving exactly like the libc function
+ * of that name. In fact, it was stolen from glibc2 and de-fancy-fied.
+ * Same semantics, slimmer shape. ;)
  */
-char * strsep(char **s, const char * ct)
+char * strsep(char **s, const char *ct)
 {
-   char *sbegin=*s;
-   if (!sbegin) 
-   return NULL;
-   
-   sbegin += strspn(sbegin,ct);
-   if (*sbegin == '\0') 
+   char *sbegin = *s;
+
+   if (sbegin == NULL)
return NULL;
-   
-   *s = strpbrk( sbegin, ct);
-   if (*s  **s != '\0')
+
+   if (*s = strpbrk(sbegin, ct))
*(*s)++ = '\0';
-   return (sbegin);
+   else
+   *s = NULL;
+
+   return sbegin;
 }
 #endif
 
--- linux-2.4.3/fs/smbfs/getopt.c   Mon Aug 14 22:31:10 2000
+++ linux-2.4.3-ac10-rs/fs/smbfs/getopt.c   Fri Apr 20 17:49:26 2001
@@ -30,8 +30,10 @@
char *val;
int i;
 
-   if ( (token = strsep(options, ",")) == NULL)
-   return 0;
+   do {
+   if ((token = strsep(options, ",")) == NULL)
+   return 0;
+   } while (*token == '\0');
*optopt = token;
 
*optarg = NULL;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/