[PATCH] [1/3] sysfs bin file fixes

From: Matthew Wilcox (willy@debian.org)
Date: Mon Apr 07 2003 - 18:42:03 EST


Completely rewrite sysfs' handling of binary files:
 - Remove sysfs_bin_buffer. It's a security hole.
 - Remove checks for permissions; the VFS does that.
 - Validate offset & count at the top level.
 - Allow lower levels to return less data than was asked for.
 - Allocate buffer at open & free it at close, not on each read/write.

diff -urpNX dontdiff linux-2.5.66/fs/sysfs/bin.c linux-2.5.66-laptop/fs/sysfs/bin.c
--- linux-2.5.66/fs/sysfs/bin.c 2003-04-07 09:59:08.000000000 -0500
+++ linux-2.5.66-laptop/fs/sysfs/bin.c 2003-04-07 11:01:19.000000000 -0500
@@ -2,168 +2,116 @@
  * bin.c - binary file operations for sysfs.
  */
 
+#include <linux/errno.h>
 #include <linux/fs.h>
-#include <linux/module.h>
 #include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/slab.h>
 
 #include "sysfs.h"
 
-static struct file_operations bin_fops;
-
-static int fill_read(struct file * file, struct sysfs_bin_buffer * buffer)
+static int
+fill_read(struct dentry *dentry, char *buffer, loff_t off, size_t count)
 {
- struct bin_attribute * attr = file->f_dentry->d_fsdata;
- struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
+ struct bin_attribute * attr = dentry->d_fsdata;
+ struct kobject * kobj = dentry->d_parent->d_fsdata;
 
- if (!buffer->data)
- attr->read(kobj,buffer);
- return buffer->size ? 0 : -ENOENT;
-}
-
-static int flush_read(struct file * file, char * userbuf,
- struct sysfs_bin_buffer * buffer)
-{
- return copy_to_user(userbuf,buffer->data + buffer->offset,buffer->count) ?
- -EFAULT : 0;
+ return attr->read(kobj, buffer, off, count);
 }
 
 static ssize_t
 read(struct file * file, char * userbuf, size_t count, loff_t * off)
 {
- struct sysfs_bin_buffer * buffer = file->private_data;
+ char *buffer = file->private_data;
+ struct dentry *dentry = file->f_dentry;
+ int size = dentry->d_inode->i_size;
+ loff_t offs = *off;
         int ret;
 
- ret = fill_read(file,buffer);
- if (ret)
- goto Done;
+ if (offs > size)
+ return 0;
+ if (offs + count > size)
+ count = size - offs;
 
- buffer->offset = *off;
+ ret = fill_read(dentry, buffer, offs, count);
+ if (ret < 0)
+ goto Done;
+ count = ret;
 
- if (count > (buffer->size - *off))
- count = buffer->size - *off;
+ ret = -EFAULT;
+ if (copy_to_user(userbuf, buffer + offs, count) != 0)
+ goto Done;
 
- buffer->count = count;
+ *off = offs + count;
+ ret = count;
 
- ret = flush_read(file,userbuf,buffer);
- if (!ret) {
- *off += count;
- ret = count;
- }
  Done:
- if (buffer && buffer->data) {
- kfree(buffer->data);
- buffer->data = NULL;
- }
         return ret;
 }
 
-int alloc_buf_data(struct sysfs_bin_buffer * buffer)
-{
- buffer->data = kmalloc(buffer->count,GFP_KERNEL);
- if (buffer->data) {
- memset(buffer->data,0,buffer->count);
- return 0;
- } else
- return -ENOMEM;
-}
-
-static int fill_write(struct file * file, const char * userbuf,
- struct sysfs_bin_buffer * buffer)
-{
- return copy_from_user(buffer->data,userbuf,buffer->count) ?
- -EFAULT : 0;
-}
-
-static int flush_write(struct file * file, const char * userbuf,
- struct sysfs_bin_buffer * buffer)
+static int
+flush_write(struct dentry *dentry, char *buffer, loff_t offset, size_t count)
 {
- struct bin_attribute * attr = file->f_dentry->d_fsdata;
- struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
+ struct bin_attribute *attr = dentry->d_fsdata;
+ struct kobject *kobj = dentry->d_parent->d_fsdata;
 
- return attr->write(kobj,buffer);
+ return attr->write(kobj, buffer, offset, count);
 }
 
 static ssize_t write(struct file * file, const char * userbuf,
                      size_t count, loff_t * off)
 {
- struct sysfs_bin_buffer * buffer = file->private_data;
+ char *buffer = file->private_data;
+ struct dentry *dentry = file->f_dentry;
+ int size = dentry->d_inode->i_size;
+ loff_t offs = *off;
         int ret;
 
- if (count > PAGE_SIZE)
- count = PAGE_SIZE;
- buffer->count = count;
-
- ret = alloc_buf_data(buffer);
- if (ret)
- goto Done;
+ if (offs > size)
+ return 0;
+ if (offs + count > size)
+ count = size - offs;
 
- ret = fill_write(file,userbuf,buffer);
- if (ret)
+ ret = -EFAULT;
+ if (copy_from_user(buffer + offs, userbuf, count))
                 goto Done;
 
- ret = flush_write(file,userbuf,buffer);
- if (ret > 0)
- *off += count;
+ count = flush_write(dentry, buffer, offs, count);
+ if (count > 0)
+ *off = offs + count;
+ ret = 0;
  Done:
- if (buffer->data) {
- kfree(buffer->data);
- buffer->data = NULL;
- }
         return ret;
 }
 
-static int check_perm(struct inode * inode, struct file * file)
+static int open(struct inode * inode, struct file * file)
 {
         struct kobject * kobj = kobject_get(file->f_dentry->d_parent->d_fsdata);
         struct bin_attribute * attr = file->f_dentry->d_fsdata;
- struct sysfs_bin_buffer * buffer;
- int error = 0;
+ int error = -EINVAL;
 
         if (!kobj || !attr)
- goto Einval;
-
- /* File needs write support.
- * The inode's perms must say it's ok,
- * and we must have a store method.
- */
- if (file->f_mode & FMODE_WRITE) {
- if (!(inode->i_mode & S_IWUGO) || !attr->write)
- goto Eaccess;
- }
-
- /* File needs read support.
- * The inode's perms must say it's ok, and we there
- * must be a show method for it.
- */
- if (file->f_mode & FMODE_READ) {
- if (!(inode->i_mode & S_IRUGO) || !attr->read)
- goto Eaccess;
- }
-
- buffer = kmalloc(sizeof(struct sysfs_bin_buffer),GFP_KERNEL);
- if (buffer) {
- memset(buffer,0,sizeof(struct sysfs_bin_buffer));
- file->private_data = buffer;
- } else
- error = -ENOMEM;
- goto Done;
+ goto Done;
 
- Einval:
- error = -EINVAL;
- goto Done;
- Eaccess:
         error = -EACCES;
+ if ((file->f_mode & FMODE_WRITE) && !attr->write)
+ goto Done;
+ if ((file->f_mode & FMODE_READ) && !attr->read)
+ goto Done;
+
+ error = -ENOMEM;
+ file->private_data = kmalloc(attr->size, GFP_KERNEL);
+ if (!file->private_data)
+ goto Done;
+
+ error = 0;
+
  Done:
         if (error && kobj)
                 kobject_put(kobj);
         return error;
 }
 
-static int open(struct inode * inode, struct file * file)
-{
- return check_perm(inode,file);
-}
-
 static int release(struct inode * inode, struct file * file)
 {
         struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
@@ -171,8 +119,7 @@ static int release(struct inode * inode,
 
         if (kobj)
                 kobject_put(kobj);
- if (buffer)
- kfree(buffer);
+ kfree(buffer);
         return 0;
 }
 
diff -urpNX dontdiff linux-2.5.66/include/linux/sysfs.h linux-2.5.66-laptop/include/linux/sysfs.h
--- linux-2.5.66/include/linux/sysfs.h 2003-04-04 08:42:19.000000000 -0600
+++ linux-2.5.66-laptop/include/linux/sysfs.h 2003-04-07 09:54:50.000000000 -0500
@@ -16,18 +16,11 @@ struct attribute {
         mode_t mode;
 };
 
-struct sysfs_bin_buffer {
- u8 * data;
- size_t size;
- size_t count;
- loff_t offset;
-};
-
 struct bin_attribute {
         struct attribute attr;
         size_t size;
- ssize_t (*read)(struct kobject *, struct sysfs_bin_buffer *);
- ssize_t (*write)(struct kobject *, struct sysfs_bin_buffer *);
+ ssize_t (*read)(struct kobject *, char *, loff_t, size_t);
+ ssize_t (*write)(struct kobject *, char *, loff_t, size_t);
 };
 
 struct sysfs_ops {

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Mon Apr 07 2003 - 22:00:33 EST