lkml.org 
[lkml]   [2000]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: Security exploit in shmfs?
From
Date
Hi Linus,

I know you did not like my first version, but what do you think about
this patch? Is it acceptable?

It resolves many of the problems we have with the shm fs code: It
works in a chrooted environment without playing with the root
directory, does not need shm_path and also no ugly trick to get it on
the fly.

It is against 99-pre1. I can add Alans stuff to make the shmat on
removed segments later.

Richard Gooch <rgooch@atnf.csiro.au> writes:
> Hi, all. It looks like there is a potential security hole in the new
> shmfs code. In ipc/shm.c:sys_shmat() the root directory is temporarily
> changed for a while. A CLONE_FS clone can then come in and take
> advantage of this exposure. This needs to be fixed.
> This is in 2.3.99-pre2.
>
> Frankly, I'd rather see the old SysV IPC code restored, especially
> since we're so close to 2.4. At least it's a known quantity.

Richard, during the whole 2.3 cycle I did test the shm code. It was
never as stable as it is today. And posix shm is something we should
really have.

Greetings
Christoph

diff -uNr 99-pre1/fs/namei.c cpre1/fs/namei.c
--- 99-pre1/fs/namei.c Mon Mar 13 10:51:32 2000
+++ cpre1/fs/namei.c Thu Mar 16 17:59:10 2000
@@ -609,13 +609,13 @@
* which is a lot more logical, and also allows the "no perm" needed
* for symlinks (where the permissions are checked later).
*/
-struct dentry * open_namei(const char * pathname, int flag, int mode)
+struct dentry * open_namei_dir(const char * pathname, int flag, int mode, struct dentry * dir)
{
int acc_mode, error;
struct inode *inode;
struct dentry *dentry;

- dentry = lookup_dentry(pathname, NULL, lookup_flags(flag));
+ dentry = lookup_dentry(pathname, dir, lookup_flags(flag));
if (IS_ERR(dentry))
return dentry;

@@ -1012,13 +1012,13 @@
return error;
}

-int do_unlink(const char * name)
+int do_unlink(const char * name, struct dentry * base)
{
int error;
struct dentry *dir;
struct dentry *dentry;

- dentry = lookup_dentry(name, NULL, 0);
+ dentry = lookup_dentry(name, base, 0);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto exit;
@@ -1043,7 +1043,7 @@
if(IS_ERR(tmp))
return PTR_ERR(tmp);
lock_kernel();
- error = do_unlink(tmp);
+ error = do_unlink(tmp, NULL);
unlock_kernel();
putname(tmp);

diff -uNr 99-pre1/fs/open.c cpre1/fs/open.c
--- 99-pre1/fs/open.c Mon Mar 13 10:51:32 2000
+++ cpre1/fs/open.c Thu Mar 16 15:55:04 2000
@@ -630,6 +630,12 @@
return error;
}

+
+struct file *filp_open(const char *name, int flags, int mode)
+{
+ return filp_open_dir(name, flags, mode, NULL);
+}
+
/*
* Note that while the flag value (low two bits) for sys_open means:
* 00 - read-only
@@ -644,7 +650,7 @@
* for the internal routines (ie open_namei()/follow_link() etc). 00 is
* used by symlinks.
*/
-struct file *filp_open(const char * filename, int flags, int mode)
+struct file *filp_open_dir(const char * filename, int flags, int mode, struct dentry *dir)
{
struct inode * inode;
struct dentry * dentry;
@@ -661,7 +667,7 @@
flag++;
if (flag & O_TRUNC)
flag |= 2;
- dentry = open_namei(filename,flag,mode);
+ dentry = open_namei_dir(filename,flag,mode,dir);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto cleanup_file;
diff -uNr 99-pre1/include/linux/fs.h cpre1/include/linux/fs.h
--- 99-pre1/include/linux/fs.h Wed Mar 15 17:33:59 2000
+++ cpre1/include/linux/fs.h Thu Mar 16 17:59:35 2000
@@ -822,6 +822,8 @@
extern void put_unused_fd(unsigned int);

extern struct file *filp_open(const char *, int, int);
+extern struct file *filp_open_dir(const char *, int, int, struct dentry*);
+
extern int filp_close(struct file *, fl_owner_t id);

extern char * getname(const char *);
@@ -936,10 +938,15 @@
extern int permission(struct inode *, int);
extern int get_write_access(struct inode *);
extern void put_write_access(struct inode *);
-extern struct dentry * open_namei(const char *, int, int);
+extern struct dentry * open_namei_dir(const char *, int, int, struct dentry *);
extern struct dentry * do_mknod(const char *, int, dev_t);
extern int do_pipe(int *);
-extern int do_unlink(const char * name);
+extern int do_unlink(const char *, struct dentry *);
+
+static inline struct dentry * open_namei(const char *name, int flag, int mode)
+{
+ return open_namei_dir(name, flag, mode, NULL);
+}

/* fs/dcache.c -- generic fs support functions */
extern int is_subdir(struct dentry *, struct dentry *);
diff -uNr 99-pre1/ipc/shm.c cpre1/ipc/shm.c
--- 99-pre1/ipc/shm.c Wed Mar 15 17:11:34 2000
+++ cpre1/ipc/shm.c Thu Mar 16 18:08:55 2000
@@ -18,10 +18,8 @@
* 1) It only can handle one directory.
* 2) Because the directory is represented by the SYSV shm array it
* can only be mounted one time.
- * 3) This again leads to SYSV shm not working properly in a chrooted
- * environment
- * 4) Read and write are not implemented (should they?)
- * 5) No special nodes are supported
+ * 3) Read and write are not implemented (should they?)
+ * 4) No special nodes are supported
*/

#include <linux/config.h>
@@ -57,8 +55,6 @@
static int shm_mmap (struct file *, struct vm_area_struct *);
static int shm_readdir (struct file *, void *, filldir_t);

-char shm_path[256] = "/var/shm";
-
#define SHM_NAME_LEN NAME_MAX
#define SHM_FMT ".IPC_%08x"
#define SHM_FMT_LEN 13
@@ -877,18 +873,6 @@
}
}

-char * shm_getname(int id)
-{
- char *result;
-
- result = __getname ();
- if (IS_ERR(result))
- return result;
-
- sprintf (result, "%s/" SHM_FMT, shm_path, id);
- return result;
-}
-
asmlinkage long sys_shmctl (int shmid, int cmd, struct shmid_ds *buf)
{
struct shm_setbuf setbuf;
@@ -1018,16 +1002,13 @@
}
case IPC_RMID:
{
- char *name;
+ char name[SHM_FMT_LEN+1];
if ((shmid % SEQ_MULTIPLIER)== zero_id)
return -EINVAL;
- name = shm_getname(shmid);
- if (IS_ERR(name))
- return PTR_ERR(name);
+ sprintf (name, SHM_FMT, shmid);
lock_kernel();
- err = do_unlink (name);
+ err = do_unlink (name, dget(shm_sb->s_root));
unlock_kernel();
- putname (name);
if (err == -ENOENT)
err = -EINVAL;
return err;
@@ -1108,7 +1089,7 @@
struct file * file;
int err;
int flags;
- char *name;
+ char name[SHM_FMT_LEN+1];

if (!shm_sb || (shmid % SEQ_MULTIPLIER) == zero_id)
return -EINVAL;
@@ -1125,13 +1106,9 @@
} else
flags = MAP_SHARED;

- name = shm_getname(shmid);
- if (IS_ERR (name))
- return PTR_ERR (name);
-
+ sprintf (name, SHM_FMT, shmid);
lock_kernel();
- file = filp_open (name, O_RDWR, 0);
- putname (name);
+ file = filp_open_dir (name, O_RDWR, 0, dget(shm_sb->s_root));
if (IS_ERR (file)) {
unlock_kernel();
goto bad_file;
@@ -1220,7 +1197,7 @@
struct page * page;

if (idx >= shp->shm_npages)
- goto sigbus;
+ return NOPAGE_SIGBUS;

pte = SHM_ENTRY(shp,idx);
if (!pte_present(pte)) {
@@ -1267,9 +1244,8 @@
return pte_page(pte);

oom:
+ shm_lock(shp->id);
return NOPAGE_OOM;
-sigbus:
- return NOPAGE_SIGBUS;
}

static struct page * shm_nopage(struct vm_area_struct * shmd, unsigned long address, int no_share)
diff -uNr 99-pre1/kernel/sysctl.c cpre1/kernel/sysctl.c
--- 99-pre1/kernel/sysctl.c Mon Mar 13 10:51:33 2000
+++ cpre1/kernel/sysctl.c Thu Mar 16 18:21:26 2000
@@ -57,7 +57,6 @@
#endif
#ifdef CONFIG_SYSVIPC
extern size_t shm_ctlmax;
-extern char shm_path[];
extern int msg_ctlmax;
extern int msg_ctlmnb;
extern int msg_ctlmni;
@@ -199,8 +198,6 @@
{KERN_RTSIGMAX, "rtsig-max", &max_queued_signals, sizeof(int),
0644, NULL, &proc_dointvec},
#ifdef CONFIG_SYSVIPC
- {KERN_SHMPATH, "shmpath", &shm_path, 256,
- 0644, NULL, &proc_dostring, &sysctl_string },
{KERN_SHMMAX, "shmmax", &shm_ctlmax, sizeof (size_t),
0644, NULL, &proc_doulongvec_minmax},
{KERN_MSGMAX, "msgmax", &msg_ctlmax, sizeof (int),
\
 
 \ /
  Last update: 2005-03-22 13:57    [W:0.197 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site