2008-12-19 20:47:39

by Duane Griffin

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

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,

2008-12-19 20:48:08

by Duane Griffin

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

Move the printk inside the !IS_ERR test.

Cc: Eric Van Hensbergen <[email protected]>
Cc: Ron Minnich <[email protected]>
Cc: Latchesar Ionkov <[email protected]>
Cc: [email protected]
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:48:27

by Duane Griffin

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

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 <[email protected]>
Cc: Dustin Kirkland <[email protected]>
Cc: [email protected]
Signed-off-by: Duane Griffin <[email protected]>
Acked-by: Michael Halcrow <[email protected]>
---

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

2008-12-19 20:48:40

by Duane Griffin

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

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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:48:57

by Duane Griffin

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

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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:49:26

by Duane Griffin

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

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

Cc: Andrew Morton <[email protected]>
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:49:43

by Duane Griffin

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

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

Cc: Andrew Morton <[email protected]>
Cc: Stephen Tweedie <[email protected]>
Cc: [email protected]
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:49:56

by Duane Griffin

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

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

Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:50:27

by Duane Griffin

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

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

Cc: Andrew Morton <[email protected]>
Cc: Theodore Ts'o <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:50:47

by Duane Griffin

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

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

Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 20:51:05

by Duane Griffin

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

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

Cc: Sergey S. Kostyliov <[email protected]>
Signed-off-by: Duane Griffin <[email protected]>
---

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

2008-12-19 22:13:33

by Eric Van Hensbergen

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

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 <[email protected]> wrote:
> Move the printk inside the !IS_ERR test.
>
> Cc: Eric Van Hensbergen <[email protected]>
> Cc: Ron Minnich <[email protected]>
> Cc: Latchesar Ionkov <[email protected]>
> Cc: [email protected]
> Signed-off-by: Duane Griffin <[email protected]>
> ---
>
> 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
>
>

2008-12-19 22:23:06

by Duane Griffin

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

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

2008-12-22 19:59:27

by Tyler Hicks

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

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 <[email protected]>

Acked-by: Tyler Hicks <[email protected]>

> Cc: Dustin Kirkland <[email protected]>
> Cc: [email protected]
> Signed-off-by: Duane Griffin <[email protected]>
> Acked-by: Michael Halcrow <[email protected]>
> ---
>
> 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;

Attachments:

2008-12-22 20:07:07

by Dustin Kirkland

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

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 <[email protected]>
> Cc: Dustin Kirkland <[email protected]>

Acked-by: Dustin Kirkland <[email protected]>

> Cc: [email protected]
> Signed-off-by: Duane Griffin <[email protected]>
> Acked-by: Michael Halcrow <[email protected]>
> ---
>
> 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;

Attachments: