[UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Take 2... all in one file.  I suppose I really did know better than 
to create that new header.  ;-)

Better?

---

CVE-2006-5753 is for a case where an inode can be marked bad, switching 
the ops to bad_inode_ops, which are all connected as:

static int return_EIO(void)
{
        return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

static struct inode_operations bad_inode_ops =
{
        .create         = bad_inode_create
...etc...

The problem here is that the void cast causes return types to not be 
promoted, and for ops such as listxattr which expect more than 32 bits of
return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
number, i.e. 0x00000000fffffffa instead of 0xfffffffa.

This goes particularly badly when the return value is taken as a number of
bytes to copy into, say, a user's buffer for example...

I originally had coded up the fix by creating a return_EIO_<TYPE> macro
for each return type, like this:

static int return_EIO_int(void)
{
	return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
	.create		= EIO_ERROR_INT,
...etc...

but Al felt that it was probably better to create an EIO-returner for each 
actual op signature.  Since so few ops share a signature, I just went ahead 
& created an EIO function for each individual file & inode op that returns
a value.

Thanks,

-Eric

Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Index: linux-2.6.19/fs/bad_inode.c
===================================================================
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -14,59 +14,308 @@
 #include <linux/time.h>
 #include <linux/smp_lock.h>
 #include <linux/namei.h>
+#include <linux/poll.h>
 
-static int return_EIO(void)
+
+static loff_t bad_file_llseek(struct file *file, loff_t offset, int origin)
+{
+	return -EIO;
+}
+
+static ssize_t bad_file_read(struct file *filp, char __user *buf,
+			size_t size, loff_t *ppos)
+{
+        return -EIO;
+}
+
+static ssize_t bad_file_write(struct file *filp, const char __user *buf,
+			size_t siz, loff_t *ppos)
+{
+        return -EIO;
+}
+
+static ssize_t bad_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
+			unsigned long nr_segs, loff_t pos)
+{
+	return -EIO;
+}
+
+static ssize_t bad_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
+			unsigned long nr_segs, loff_t pos)
+{
+	return -EIO;
+}
+
+static int bad_file_readdir(struct file * filp, void * dirent,
+			filldir_t filldir)
+{
+	return -EIO;
+}
+
+static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+	return -EIO;
+}
+
+static int bad_file_ioctl (struct inode * inode, struct file * filp,
+			unsigned int cmd, unsigned long arg)
 {
 	return -EIO;
 }
 
-#define EIO_ERROR ((void *) (return_EIO))
+static long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+			unsigned long arg)
+{
+	return -EIO;
+}
+
+static long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+			unsigned long arg)
+{
+	return -EIO;
+}
+
+static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+{
+	return -EIO;
+}
+
+static int bad_file_open(struct inode * inode, struct file * filp)
+{
+	return -EIO;
+}
+
+static int bad_file_flush(struct file *file, fl_owner_t id)
+{
+	return -EIO;
+}
+
+static int bad_file_release(struct inode * inode, struct file * filp)
+{
+	return -EIO;
+}
+
+static int bad_file_fsync(struct file * file, struct dentry *dentry,
+			int datasync)
+{
+	return -EIO;
+}
+
+static int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+{
+	return -EIO;
+}
+
+static int bad_file_fasync(int fd, struct file *filp, int on)
+{
+	return -EIO;
+}
+
+static int bad_file_lock(struct file *file, int cmd, struct file_lock *fl)
+{
+	return -EIO;
+}
+
+static ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+			size_t count, read_actor_t actor, void *target)
+{
+	return -EIO;
+}
+
+static ssize_t bad_file_sendpage(struct file *file, struct page *page,
+			int off, size_t len, loff_t *pos, int more)
+{
+	return -EIO;
+}
+
+static unsigned long bad_file_get_unmapped_area(struct file *file,
+				unsigned long addr, unsigned long len,
+				unsigned long pgoff, unsigned long flags)
+{
+	return -EIO;
+}
+
+static int bad_file_check_flags(int flags)
+{
+	return -EIO;
+}
+
+static int bad_file_dir_notify(struct file * file, unsigned long arg)
+{
+	return -EIO;
+}
+
+static int bad_file_flock(struct file *filp, int cmd, struct file_lock *fl)
+{
+	return -EIO;
+}
+
+static ssize_t bad_file_splice_write(struct pipe_inode_info *pipe,
+			struct file *out, loff_t *ppos, size_t len,
+			unsigned int flags)
+{
+	return -EIO;
+}
+
+static ssize_t bad_file_splice_read(struct file *in, loff_t *ppos,
+			struct pipe_inode_info *pipe, size_t len,
+			unsigned int flags)
+{
+	return -EIO;
+}
 
 static const struct file_operations bad_file_ops =
 {
-	.llseek		= EIO_ERROR,
-	.aio_read	= EIO_ERROR,
-	.read		= EIO_ERROR,
-	.write		= EIO_ERROR,
-	.aio_write	= EIO_ERROR,
-	.readdir	= EIO_ERROR,
-	.poll		= EIO_ERROR,
-	.ioctl		= EIO_ERROR,
-	.mmap		= EIO_ERROR,
-	.open		= EIO_ERROR,
-	.flush		= EIO_ERROR,
-	.release	= EIO_ERROR,
-	.fsync		= EIO_ERROR,
-	.aio_fsync	= EIO_ERROR,
-	.fasync		= EIO_ERROR,
-	.lock		= EIO_ERROR,
-	.sendfile	= EIO_ERROR,
-	.sendpage	= EIO_ERROR,
-	.get_unmapped_area = EIO_ERROR,
+	.llseek		= bad_file_llseek,
+	.read		= bad_file_read,
+	.write		= bad_file_write,
+	.aio_read	= bad_file_aio_read,
+	.aio_write	= bad_file_aio_write,
+	.readdir	= bad_file_readdir,
+	.poll		= bad_file_poll,
+	.ioctl		= bad_file_ioctl,
+	.unlocked_ioctl	= bad_file_unlocked_ioctl,
+	.compat_ioctl	= bad_file_compat_ioctl,
+	.mmap		= bad_file_mmap,
+	.open		= bad_file_open,
+	.flush		= bad_file_flush,
+	.release	= bad_file_release,
+	.fsync		= bad_file_fsync,
+	.aio_fsync	= bad_file_aio_fsync,
+	.fasync		= bad_file_fasync,
+	.lock		= bad_file_lock,
+	.sendfile	= bad_file_sendfile,
+	.sendpage	= bad_file_sendpage,
+	.get_unmapped_area = bad_file_get_unmapped_area,
+	.check_flags	= bad_file_check_flags,
+	.dir_notify	= bad_file_dir_notify,
+	.flock		= bad_file_flock,
+	.splice_write	= bad_file_splice_write,
+	.splice_read	= bad_file_splice_read,
 };
 
+static int bad_inode_create (struct inode * dir, struct dentry * dentry,
+		int mode, struct nameidata *nd)
+{
+	return -EIO;
+}
+
+static struct dentry *bad_inode_lookup(struct inode * dir,
+			struct dentry *dentry, struct nameidata *nd)
+{
+	return ERR_PTR(-EIO);
+}
+
+static int bad_inode_link (struct dentry * old_dentry, struct inode * dir,
+		struct dentry *dentry)
+{
+	return -EIO;
+}
+
+static int bad_inode_unlink(struct inode * dir, struct dentry *dentry)
+{
+	return -EIO;
+}
+
+static int bad_inode_symlink (struct inode * dir, struct dentry *dentry,
+		const char * symname)
+{
+	return -EIO;
+}
+
+static int bad_inode_mkdir(struct inode * dir, struct dentry * dentry,
+			int mode)
+{
+	return -EIO;
+}
+
+static int bad_inode_rmdir (struct inode * dir, struct dentry *dentry)
+{
+	return -EIO;
+}
+
+static int bad_inode_mknod (struct inode * dir, struct dentry *dentry,
+			int mode, dev_t rdev)
+{
+	return -EIO;
+}
+
+static int bad_inode_rename (struct inode * old_dir, struct dentry *old_dentry,
+		struct inode * new_dir, struct dentry *new_dentry)
+{
+	return -EIO;
+}
+
+static int bad_inode_readlink(struct dentry *dentry, char __user *buffer,
+		int buflen)
+{
+	return -EIO;
+}
+
+static int bad_inode_permission(struct inode *inode, int mask,
+			struct nameidata *nd)
+{
+	return -EIO;
+}
+
+static int bad_inode_getattr(struct vfsmount *mnt, struct dentry *dentry,
+			struct kstat *stat)
+{
+	return -EIO;
+}
+
+static int bad_inode_setattr(struct dentry *direntry, struct iattr *attrs)
+{
+	return -EIO;
+}
+
+static int bad_inode_setxattr(struct dentry *dentry, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return -EIO;
+}
+
+static ssize_t bad_inode_getxattr(struct dentry *dentry, const char *name,
+			void *buffer, size_t size)
+{
+	return -EIO;
+}
+
+static ssize_t bad_inode_listxattr(struct dentry *dentry, char *buffer,
+			size_t buffer_size)
+{
+	return -EIO;
+}
+
+static int bad_inode_removexattr(struct dentry *dentry, const char *name)
+{
+	return -EIO;
+}
+
 static struct inode_operations bad_inode_ops =
 {
-	.create		= EIO_ERROR,
-	.lookup		= EIO_ERROR,
-	.link		= EIO_ERROR,
-	.unlink		= EIO_ERROR,
-	.symlink	= EIO_ERROR,
-	.mkdir		= EIO_ERROR,
-	.rmdir		= EIO_ERROR,
-	.mknod		= EIO_ERROR,
-	.rename		= EIO_ERROR,
-	.readlink	= EIO_ERROR,
+	.create		= bad_inode_create,
+	.lookup		= bad_inode_lookup,
+	.link		= bad_inode_link,
+	.unlink		= bad_inode_unlink,
+	.symlink	= bad_inode_symlink,
+	.mkdir		= bad_inode_mkdir,
+	.rmdir		= bad_inode_rmdir,
+	.mknod		= bad_inode_mknod,
+	.rename		= bad_inode_rename,
+	.readlink	= bad_inode_readlink,
 	/* follow_link must be no-op, otherwise unmounting this inode
 	   won't work */
-	.truncate	= EIO_ERROR,
-	.permission	= EIO_ERROR,
-	.getattr	= EIO_ERROR,
-	.setattr	= EIO_ERROR,
-	.setxattr	= EIO_ERROR,
-	.getxattr	= EIO_ERROR,
-	.listxattr	= EIO_ERROR,
-	.removexattr	= EIO_ERROR,
+	/* put_link returns void */
+	/* truncate returns void */
+	.permission	= bad_inode_permission,
+	.getattr	= bad_inode_getattr,
+	.setattr	= bad_inode_setattr,
+	.setxattr	= bad_inode_setxattr,
+	.getxattr	= bad_inode_getxattr,
+	.listxattr	= bad_inode_listxattr,
+	.removexattr	= bad_inode_removexattr,
+	/* truncate_range returns void */
 };
 
 

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Eric Sandeen wrote:
> Take 2... all in one file.  I suppose I really did know better than 
> to create that new header.  ;-)
> 
> Better?

Ugh, sorry for the internal/external cross-post, that was supposed to be
to LKML... not enough sleep last night.  Please adjust replies
accordingly.  :(

-Eric

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Phillip Lougher]

Eric Sandeen wrote:
>but Al felt that it was probably better to create an EIO-returner for each 
>actual op signature.  Since so few ops share a signature, I just went ahead 
>& created an EIO function for each individual file & inode op that returns
>a value.

Hmm, the problem with this is it bloats bad_inode.o with lots of empty
functions that return -EIO.  Even though we're not interested in the
parameters, GCC doesn't know this, and doesn't fold the functions into only
the couple of definitions that return different types.

Text size of original bad_inode.o:

Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         0000006c  00000000  00000000  00000034  2**2

== 108 bytes

Size with patch applied:

Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         0000016b  00000000  00000000  00000034  2**2 patch
applied:

== 363 bytes, or over three times larger!

>I originally had coded up the fix by creating a return_EIO_<TYPE> macro 
>for each return type,

This adds two extra functions (return for ssize_t and long), which gives an
increase in size of only 12 bytes:

Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00000078  00000000  00000000  00000034  2**2

== 120 bytes.

Isn't this better?

Thanks

Phillip

-- 
View this message in context: http://www.nabble.com/-UPDATED-PATCH--fix-memory-corruption-from-misinterpreted-bad_inode_ops-return-values-tf2916716.html#a8178968
Sent from the linux-fsdevel mailing list archive at Nabble.com.

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[phillip]

phillip@lougher.demon.co.uk wrote:
> Hmm, the problem with this is it bloats bad_inode.o with lots of empty
> functions that return -EIO.  Even though we're not interested in the
> parameters, GCC doesn't know this, and doesn't fold the functions into only
> the couple of definitions that return different types.
> 

After posting this, I saw the discussion on the LKML.  I was only about 12 hours late, as usual...

Phillip