[PATCH 0/10] make link target handling more robust

[PATCH 0/10] make link target handling more robust

[Duane Griffin]

These patches fix potential bugs associated with link target handling,
primarily by NUL-terminating names read from disk.

This is version 5 of these patches. This version fixes an off-by-one
bug, pointed out by Andreas Dilger, where the maximum name length was
calculated with sizeof without leaving space for the terminator.

This set includes patches for all affected filesystems except for jfs,
which has already been fixed by David Kleikamp, and ufs, where I am
still discussing some issues with the maintainer. I'll post the ufs
patch separately once it is ready.

The eCryptFS fix should be considered for stable.

diffstat:
 fs/9p/vfs_inode.c        |    5 +++--
 fs/befs/linuxvfs.c       |    5 ++++-
 fs/ecryptfs/inode.c      |    3 ++-
 fs/ext2/inode.c          |    7 +++++--
 fs/ext3/inode.c          |    7 +++++--
 fs/ext4/inode.c          |    7 +++++--
 fs/freevxfs/vxfs_inode.c |    4 +++-
 fs/namei.c               |    7 +++++--
 fs/sysv/inode.c          |    6 +++++-
 include/linux/namei.h    |    5 +++++
 10 files changed, 42 insertions(+), 14 deletions(-)

Cheers,
Duane,

[PATCH, v5] 9p: don't print IS_ERR strings

[Duane Griffin]

Move the printk inside the !IS_ERR test.

Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Ron Minnich <rminnich@sandia.gov>
Cc: Latchesar Ionkov <lucho@ionkov.net>
Cc: v9fs-developer@lists.sourceforge.net
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Unchanged from original version.

 fs/9p/vfs_inode.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 8fddfe8..c50d555 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1022,9 +1022,10 @@ v9fs_vfs_put_link(struct dentry *dentry, struct nameidata *nd, void *p)
 {
 	char *s = nd_get_link(nd);
 
-	P9_DPRINTK(P9_DEBUG_VFS, " %s %s\n", dentry->d_name.name, s);
-	if (!IS_ERR(s))
+	if (!IS_ERR(s)) {
+		P9_DPRINTK(P9_DEBUG_VFS, " %s %s\n", dentry->d_name.name, s);
 		__putname(s);
+	}
 }
 
 /**
-- 
1.6.0.4

[PATCH, v5] eCryptfs: check readlink result was not an error before using it

[Duane Griffin]

The result from readlink is being used to index into the link name
buffer without checking whether it is a valid length. If readlink
returns an error this will fault or cause memory corruption.

Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
Cc: Dustin Kirkland <kirkland@canonical.com>
Cc: ecryptfs-devel@lists.launchpad.net
Signed-off-by: Duane Griffin <duaneg@dghda.com>
Acked-by: Michael Halcrow <mhalcrow@us.ibm.com>
---

Unchanged from original version.

 fs/ecryptfs/inode.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 89209f0..5e78fc1 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -673,10 +673,11 @@ static void *ecryptfs_follow_link(struct dentry *dentry, struct nameidata *nd)
 	ecryptfs_printk(KERN_DEBUG, "Calling readlink w/ "
 			"dentry->d_name.name = [%s]\n", dentry->d_name.name);
 	rc = dentry->d_inode->i_op->readlink(dentry, (char __user *)buf, len);
-	buf[rc] = '\0';
 	set_fs(old_fs);
 	if (rc < 0)
 		goto out_free;
+	else
+		buf[rc] = '\0';
 	rc = 0;
 	nd_set_link(nd, buf);
 	goto out;
-- 
1.6.0.4

[PATCH, v5] vfs: introduce helper function to safely NUL-terminate symlinks

[Duane Griffin]

A number of filesystems were potentially triggering kernel bugs due to
corrupted symlink names on disk. This function helps safely terminate
the names.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Unchanged from v4.

 include/linux/namei.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/linux/namei.h b/include/linux/namei.h
index 99eb803..fc2e035 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -94,4 +94,9 @@ static inline char *nd_get_link(struct nameidata *nd)
 	return nd->saved_names[nd->depth];
 }
 
+static inline void nd_terminate_link(void *name, size_t len, size_t maxlen)
+{
+	((char *) name)[min(len, maxlen)] = '\0';
+}
+
 #endif /* _LINUX_NAMEI_H */
-- 
1.6.0.4

[PATCH, v5] vfs: ensure page symlinks are NUL-terminated

[Duane Griffin]

On-disk data corruption could cause a page link to have its i_size set
to PAGE_SIZE (or a multiple thereof) and its contents all non-NUL.
NUL-terminate the link name to ensure this doesn't cause further
problems for the kernel.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Unchanged from v4.

 fs/namei.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index af3783f..2416922 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2750,13 +2750,16 @@ int vfs_follow_link(struct nameidata *nd, const char *link)
 /* get the link contents into pagecache */
 static char *page_getlink(struct dentry * dentry, struct page **ppage)
 {
-	struct page * page;
+	char *kaddr;
+	struct page *page;
 	struct address_space *mapping = dentry->d_inode->i_mapping;
 	page = read_mapping_page(mapping, 0, NULL);
 	if (IS_ERR(page))
 		return (char*)page;
 	*ppage = page;
-	return kmap(page);
+	kaddr = kmap(page);
+	nd_terminate_link(kaddr, dentry->d_inode->i_size, PAGE_SIZE - 1);
+	return kaddr;
 }
 
 int page_readlink(struct dentry *dentry, char __user *buffer, int buflen)
-- 
1.6.0.4

[PATCH, v5] ext2: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Changes from v4: fixed off-by-one bug in maximum length, as pointed out by
Andreas Dilger.

 fs/ext2/inode.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 7658b33..02b39a5 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -32,6 +32,7 @@
 #include <linux/buffer_head.h>
 #include <linux/mpage.h>
 #include <linux/fiemap.h>
+#include <linux/namei.h>
 #include "ext2.h"
 #include "acl.h"
 #include "xip.h"
@@ -1286,9 +1287,11 @@ struct inode *ext2_iget (struct super_block *sb, unsigned long ino)
 		else
 			inode->i_mapping->a_ops = &ext2_aops;
 	} else if (S_ISLNK(inode->i_mode)) {
-		if (ext2_inode_is_fast_symlink(inode))
+		if (ext2_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext2_fast_symlink_inode_operations;
-		else {
+			nd_terminate_link(ei->i_data, inode->i_size,
+				sizeof(ei->i_data) - 1);
+		} else {
 			inode->i_op = &ext2_symlink_inode_operations;
 			if (test_opt(inode->i_sb, NOBH))
 				inode->i_mapping->a_ops = &ext2_nobh_aops;
-- 
1.6.0.4

[PATCH, v5] ext3: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Stephen Tweedie <sct@redhat.com>
Cc: linux-ext4@vger.kernel.org
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Changes from v4: fixed off-by-one bug in maximum length, as pointed out by
Andreas Dilger.

 fs/ext3/inode.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index f8424ad..c4bdccf 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -37,6 +37,7 @@
 #include <linux/uio.h>
 #include <linux/bio.h>
 #include <linux/fiemap.h>
+#include <linux/namei.h>
 #include "xattr.h"
 #include "acl.h"
 
@@ -2817,9 +2818,11 @@ struct inode *ext3_iget(struct super_block *sb, unsigned long ino)
 		inode->i_op = &ext3_dir_inode_operations;
 		inode->i_fop = &ext3_dir_operations;
 	} else if (S_ISLNK(inode->i_mode)) {
-		if (ext3_inode_is_fast_symlink(inode))
+		if (ext3_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext3_fast_symlink_inode_operations;
-		else {
+			nd_terminate_link(ei->i_data, inode->i_size,
+				sizeof(ei->i_data) - 1);
+		} else {
 			inode->i_op = &ext3_symlink_inode_operations;
 			ext3_set_aops(inode);
 		}
-- 
1.6.0.4

[PATCH, v5] ext4: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: adilger@sun.com
Cc: linux-ext4@vger.kernel.org
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Changes from v4: fixed off-by-one bug in maximum length, as pointed out by
Andreas Dilger.

 fs/ext4/inode.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ca88060..f940859 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -34,6 +34,7 @@
 #include <linux/writeback.h>
 #include <linux/pagevec.h>
 #include <linux/mpage.h>
+#include <linux/namei.h>
 #include <linux/uio.h>
 #include <linux/bio.h>
 #include "ext4_jbd2.h"
@@ -4200,9 +4201,11 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 		inode->i_op = &ext4_dir_inode_operations;
 		inode->i_fop = &ext4_dir_operations;
 	} else if (S_ISLNK(inode->i_mode)) {
-		if (ext4_inode_is_fast_symlink(inode))
+		if (ext4_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-		else {
+			nd_terminate_link(ei->i_data, inode->i_size,
+				sizeof(ei->i_data) - 1);
+		} else {
 			inode->i_op = &ext4_symlink_inode_operations;
 			ext4_set_aops(inode);
 		}
-- 
1.6.0.4

[PATCH, v5] sysv: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Changes from v4: fixed off-by-one bug in maximum length, as pointed out by
Andreas Dilger.

 fs/sysv/inode.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index df0d435..3d81bf5 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -27,6 +27,7 @@
 #include <linux/init.h>
 #include <linux/buffer_head.h>
 #include <linux/vfs.h>
+#include <linux/namei.h>
 #include <asm/byteorder.h>
 #include "sysv.h"
 
@@ -163,8 +164,11 @@ void sysv_set_inode(struct inode *inode, dev_t rdev)
 		if (inode->i_blocks) {
 			inode->i_op = &sysv_symlink_inode_operations;
 			inode->i_mapping->a_ops = &sysv_aops;
-		} else
+		} else {
 			inode->i_op = &sysv_fast_symlink_inode_operations;
+			nd_terminate_link(SYSV_I(inode)->i_data, inode->i_size,
+				sizeof(SYSV_I(inode)->i_data) - 1);
+		}
 	} else
 		init_special_inode(inode, inode->i_mode, rdev);
 }
-- 
1.6.0.4

[PATCH, v5] freevxfs: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Unchanged from v4.

 fs/freevxfs/vxfs_inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/freevxfs/vxfs_inode.c b/fs/freevxfs/vxfs_inode.c
index 9f3f2ce..03a6ea5 100644
--- a/fs/freevxfs/vxfs_inode.c
+++ b/fs/freevxfs/vxfs_inode.c
@@ -325,8 +325,10 @@ vxfs_iget(struct super_block *sbp, ino_t ino)
 		if (!VXFS_ISIMMED(vip)) {
 			ip->i_op = &page_symlink_inode_operations;
 			ip->i_mapping->a_ops = &vxfs_aops;
-		} else
+		} else {
 			ip->i_op = &vxfs_immed_symlink_iops;
+			vip->vii_immed.vi_immed[ip->i_size] = '\0';
+		}
 	} else
 		init_special_inode(ip, ip->i_mode, old_decode_dev(vip->vii_rdev));
 
-- 
1.6.0.4

[PATCH, v5] befs: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Cc: Sergey S. Kostyliov <rathamahata@php4.ru>
Signed-off-by: Duane Griffin <duaneg@dghda.com>
---

Unchanged from original version.

 fs/befs/linuxvfs.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index b6dfee3..d06cb02 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -378,7 +378,8 @@ static struct inode *befs_iget(struct super_block *sb, unsigned long ino)
 		inode->i_size = 0;
 		inode->i_blocks = befs_sb->block_size / VFS_BLOCK_SIZE;
 		strncpy(befs_ino->i_data.symlink, raw_inode->data.symlink,
-			BEFS_SYMLINK_LEN);
+			BEFS_SYMLINK_LEN - 1);
+		befs_ino->i_data.symlink[BEFS_SYMLINK_LEN - 1] = '\0';
 	} else {
 		int num_blks;
 
@@ -477,6 +478,8 @@ befs_follow_link(struct dentry *dentry, struct nameidata *nd)
 			kfree(link);
 			befs_error(sb, "Failed to read entire long symlink");
 			link = ERR_PTR(-EIO);
+		} else {
+			link[len - 1] = '\0';
 		}
 	} else {
 		link = befs_ino->i_data.symlink;
-- 
1.6.0.4

Re: [PATCH, v5] 9p: don't print IS_ERR strings

[Eric Van Hensbergen]

NAK - the print is a debug to mark function entry when debugging is on
-- it is not intended to show only success.  If an erroneous s will
cause the print to break then perhaps it should be parameterized, but
the entire print shouldn't be pushed inside the if statement.

            -eric


On Fri, Dec 19, 2008 at 2:47 PM, Duane Griffin <duaneg@dghda.com> wrote:
> Move the printk inside the !IS_ERR test.
>
> Cc: Eric Van Hensbergen <ericvh@gmail.com>
> Cc: Ron Minnich <rminnich@sandia.gov>
> Cc: Latchesar Ionkov <lucho@ionkov.net>
> Cc: v9fs-developer@lists.sourceforge.net
> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> ---
>
> Unchanged from original version.
>
>  fs/9p/vfs_inode.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 8fddfe8..c50d555 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1022,9 +1022,10 @@ v9fs_vfs_put_link(struct dentry *dentry, struct nameidata *nd, void *p)
>  {
>        char *s = nd_get_link(nd);
>
> -       P9_DPRINTK(P9_DEBUG_VFS, " %s %s\n", dentry->d_name.name, s);
> -       if (!IS_ERR(s))
> +       if (!IS_ERR(s)) {
> +               P9_DPRINTK(P9_DEBUG_VFS, " %s %s\n", dentry->d_name.name, s);
>                __putname(s);
> +       }
>  }
>
>  /**
> --
> 1.6.0.4
>
>

Re: [PATCH, v5] 9p: don't print IS_ERR strings

[Duane Griffin]

On Fri, Dec 19, 2008 at 04:07:53PM -0600, Eric Van Hensbergen wrote:
> NAK - the print is a debug to mark function entry when debugging is on
> -- it is not intended to show only success.  If an erroneous s will
> cause the print to break then perhaps it should be parameterized, but
> the entire print shouldn't be pushed inside the if statement.

If s is an error then I suppose it will BUG when the printk tries to
dereference it. So I think a fix is necessary. How about this?

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 8fddfe8..e10c27d 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1022,7 +1022,9 @@ v9fs_vfs_put_link(struct dentry *dentry, struct nameidata *nd, void *p)
 {
 	char *s = nd_get_link(nd);
 
-	P9_DPRINTK(P9_DEBUG_VFS, " %s %s\n", dentry->d_name.name, s);
+	P9_DPRINTK(P9_DEBUG_VFS, " %s %s\n", dentry->d_name.name,
+			IS_ERR(s) ? "<error>" : s);
+
 	if (!IS_ERR(s))
 		__putname(s);
 }

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH, v5] eCryptfs: check readlink result was not an error before using it

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

Duane Griffin wrote:
> The result from readlink is being used to index into the link name
> buffer without checking whether it is a valid length. If readlink
> returns an error this will fault or cause memory corruption.
> 
> Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>

Acked-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com>

> Cc: Dustin Kirkland <kirkland@canonical.com>
> Cc: ecryptfs-devel@lists.launchpad.net
> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> Acked-by: Michael Halcrow <mhalcrow@us.ibm.com>
> ---
> 
> Unchanged from original version.
> 
>  fs/ecryptfs/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 89209f0..5e78fc1 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -673,10 +673,11 @@ static void *ecryptfs_follow_link(struct dentry *dentry, struct nameidata *nd)
>  	ecryptfs_printk(KERN_DEBUG, "Calling readlink w/ "
>  			"dentry->d_name.name = [%s]\n", dentry->d_name.name);
>  	rc = dentry->d_inode->i_op->readlink(dentry, (char __user *)buf, len);
> -	buf[rc] = '\0';
>  	set_fs(old_fs);
>  	if (rc < 0)
>  		goto out_free;
> +	else
> +		buf[rc] = '\0';
>  	rc = 0;
>  	nd_set_link(nd, buf);
>  	goto out;



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH, v5] eCryptfs: check readlink result was not an error before using it

[Dustin Kirkland]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On Fri, 2008-12-19 at 20:47 +0000, Duane Griffin wrote:
> The result from readlink is being used to index into the link name
> buffer without checking whether it is a valid length. If readlink
> returns an error this will fault or cause memory corruption.
> 
> Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
> Cc: Dustin Kirkland <kirkland@canonical.com>

Acked-by: Dustin Kirkland <kirkland@canonical.com>

> Cc: ecryptfs-devel@lists.launchpad.net
> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> Acked-by: Michael Halcrow <mhalcrow@us.ibm.com>
> ---
> 
> Unchanged from original version.
> 
>  fs/ecryptfs/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 89209f0..5e78fc1 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -673,10 +673,11 @@ static void *ecryptfs_follow_link(struct dentry *dentry, struct nameidata *nd)
>  	ecryptfs_printk(KERN_DEBUG, "Calling readlink w/ "
>  			"dentry->d_name.name = [%s]\n", dentry->d_name.name);
>  	rc = dentry->d_inode->i_op->readlink(dentry, (char __user *)buf, len);
> -	buf[rc] = '\0';
>  	set_fs(old_fs);
>  	if (rc < 0)
>  		goto out_free;
> +	else
> +		buf[rc] = '\0';
>  	rc = 0;
>  	nd_set_link(nd, buf);
>  	goto out;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]