* [PATCH 0/8, v3] ensure symlinks are NUL-terminated @ 2008-12-16 15:51 Duane Griffin 2008-12-16 15:51 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Duane Griffin 2008-12-16 17:03 ` [PATCH 0/8, v3] ensure symlinks are NUL-terminated Dave Kleikamp 0 siblings, 2 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Andrew Morton, Al Viro, Evgeniy Dushistov These patches fix potential bugs associated with link target handling by NUL-terminating names read from disk. This is version 3 of these patches. It fixes the bug, pointed out by Al Viro and Evgeniy Dushistov, that i_size was not being validated. In order to facilitate this it introduces a helper function for terminating the link name, as suggested by Al. diffstat: 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 +++++- fs/ufs/inode.c | 8 ++++++-- include/linux/namei.h | 5 +++++ 8 files changed, 39 insertions(+), 12 deletions(-) ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-16 15:51 [PATCH 0/8, v3] ensure symlinks are NUL-terminated Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:51 ` [PATCH] vfs: ensure page symlinks are NUL-terminated Duane Griffin 2008-12-16 16:38 ` Al Viro 2008-12-16 17:03 ` [PATCH 0/8, v3] ensure symlinks are NUL-terminated Dave Kleikamp 1 sibling, 2 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Duane Griffin, Al Viro, Andrew Morton A number of filesystems were potentially triggering kernel bugs due to corrupted symlink names on disk. This helper 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> --- 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..231ce28 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, unsigned len, unsigned maxlen) +{ + ((char *) name)[min(len, maxlen)] = '\0'; +} + #endif /* _LINUX_NAMEI_H */ -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] vfs: ensure page symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:51 ` [PATCH] ext2: ensure fast " Duane Griffin 2008-12-16 16:38 ` Al Viro 1 sibling, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Duane Griffin, Al Viro, Andrew Morton 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> --- 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] ext2: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] vfs: ensure page symlinks are NUL-terminated Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:51 ` [PATCH] ext3: " Duane Griffin 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Duane Griffin, Andrew Morton 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> --- 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..c902504 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)); + } 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] ext3: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] ext2: ensure fast " Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:51 ` [PATCH] ext4: " Duane Griffin 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Duane Griffin, Andrew Morton, Stephen Tweedie, linux-ext4 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> --- 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..24aacdf 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)); + } else { inode->i_op = &ext3_symlink_inode_operations; ext3_set_aops(inode); } -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] ext4: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] ext3: " Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:51 ` [PATCH] ufs: " Duane Griffin 2008-12-16 23:46 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Andreas Dilger 0 siblings, 2 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Duane Griffin, Andrew Morton, Theodore Ts'o, adilger, linux-ext4 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> --- 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..f0e6bfb 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)); + } else { inode->i_op = &ext4_symlink_inode_operations; ext4_set_aops(inode); } -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] ufs: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] ext4: " Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:51 ` [PATCH] sysv: " Duane Griffin 2008-12-16 19:40 ` [PATCH] ufs: " Evgeniy Dushistov 2008-12-16 23:46 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Andreas Dilger 1 sibling, 2 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Duane Griffin, Evgeniy Dushistov Ensure fast symlink targets are NUL-terminated, even if corrupted on-disk. Cc: Evgeniy Dushistov <dushistov@mail.ru> Signed-off-by: Duane Griffin <duaneg@dghda.com> --- fs/ufs/inode.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c index 39f8778..aca4b58 100644 --- a/fs/ufs/inode.c +++ b/fs/ufs/inode.c @@ -36,6 +36,7 @@ #include <linux/mm.h> #include <linux/smp_lock.h> #include <linux/buffer_head.h> +#include <linux/namei.h> #include "ufs_fs.h" #include "ufs.h" @@ -606,9 +607,12 @@ static void ufs_set_inode_ops(struct inode *inode) inode->i_fop = &ufs_dir_operations; inode->i_mapping->a_ops = &ufs_aops; } else if (S_ISLNK(inode->i_mode)) { - if (!inode->i_blocks) + if (!inode->i_blocks) { inode->i_op = &ufs_fast_symlink_inode_operations; - else { + nd_terminate_link(UFS_I(inode)->i_u1.i_symlink, + inode->i_size, + sizeof(UFS_I(inode)->i_u1.i_symlink)); + } else { inode->i_op = &page_symlink_inode_operations; inode->i_mapping->a_ops = &ufs_aops; } -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] sysv: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] ufs: " Duane Griffin @ 2008-12-16 15:51 ` Duane Griffin 2008-12-16 15:52 ` [PATCH] freevxfs: " Duane Griffin 2008-12-16 19:40 ` [PATCH] ufs: " Evgeniy Dushistov 1 sibling, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:51 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Duane Griffin, Christoph Hellwig, Al Viro 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> --- 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..c93189d 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)); + } } else init_special_inode(inode, inode->i_mode, rdev); } -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] freevxfs: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] sysv: " Duane Griffin @ 2008-12-16 15:52 ` Duane Griffin 0 siblings, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-16 15:52 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Duane Griffin, Christoph Hellwig 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> --- 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] ufs: ensure fast symlinks are NUL-terminated 2008-12-16 15:51 ` [PATCH] ufs: " Duane Griffin 2008-12-16 15:51 ` [PATCH] sysv: " Duane Griffin @ 2008-12-16 19:40 ` Evgeniy Dushistov 2008-12-16 23:18 ` Duane Griffin 1 sibling, 1 reply; 20+ messages in thread From: Evgeniy Dushistov @ 2008-12-16 19:40 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel On Tue, Dec 16, 2008 at 03:51:58PM +0000, Duane Griffin wrote: > Ensure fast symlink targets are NUL-terminated, even if corrupted > on-disk. > > Cc: Evgeniy Dushistov <dushistov@mail.ru> > Signed-off-by: Duane Griffin <duaneg@dghda.com> > --- > fs/ufs/inode.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c > index 39f8778..aca4b58 100644 > --- a/fs/ufs/inode.c > +++ b/fs/ufs/inode.c > @@ -36,6 +36,7 @@ > #include <linux/mm.h> > #include <linux/smp_lock.h> > #include <linux/buffer_head.h> > +#include <linux/namei.h> > > #include "ufs_fs.h" > #include "ufs.h" > @@ -606,9 +607,12 @@ static void ufs_set_inode_ops(struct inode *inode) > inode->i_fop = &ufs_dir_operations; > inode->i_mapping->a_ops = &ufs_aops; > } else if (S_ISLNK(inode->i_mode)) { > - if (!inode->i_blocks) > + if (!inode->i_blocks) { > inode->i_op = &ufs_fast_symlink_inode_operations; > - else { > + nd_terminate_link(UFS_I(inode)->i_u1.i_symlink, > + inode->i_size, > + sizeof(UFS_I(inode)->i_u1.i_symlink)); > + } else { > inode->i_op = &page_symlink_inode_operations; > inode->i_mapping->a_ops = &ufs_aops; > } > -- > 1.6.0.4 There is different types of ufs, one used 64 bit for "pointers to blocks", another 32 bit, so sizeof(UFS_I(inode)->i_u1.i_symlink)) is not right choice every time, in ufs2 it should be sizeof(UFS_I(inode)->i_u1.u2_i_data) which 2 times bigger, also there is hint for *BSD ufs fs/ufs/ufs_fs.h: __fs32 fs_maxsymlinklen;/* max length of an internal symlink */ which may be used if ufs type ufs1 or ufs2 -- /Evgeniy ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] ufs: ensure fast symlinks are NUL-terminated 2008-12-16 19:40 ` [PATCH] ufs: " Evgeniy Dushistov @ 2008-12-16 23:18 ` Duane Griffin 2008-12-27 12:22 ` Evgeniy Dushistov 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-16 23:18 UTC (permalink / raw) To: Evgeniy Dushistov; +Cc: linux-kernel, linux-fsdevel On Tue, Dec 16, 2008 at 10:40:55PM +0300, Evgeniy Dushistov wrote: > There is different types of ufs, one used 64 bit for "pointers to > blocks", another 32 bit, > so sizeof(UFS_I(inode)->i_u1.i_symlink)) > is not right choice every time, > in ufs2 it should be > sizeof(UFS_I(inode)->i_u1.u2_i_data) which 2 times bigger, > > also there is hint for *BSD ufs > > fs/ufs/ufs_fs.h: > __fs32 fs_maxsymlinklen;/* max length of an internal symlink */ > > which may be used if ufs type ufs1 or ufs2 Hmm, I see. However it looks like ufs1_read_inode and ufs2_read_inode both copy the same, ((UFS_NDADDR + UFS_NINDIR) * 4), amount of inline symlink data. They also both copy it to ufs_inode_info->i_u1.i_symlink (not that that matters, I suppose). Perhaps I'm being obtuse, but it looks like inline ufs2 symlinks between 60 and 120 characters long are being truncated to 60 characters, no? There also doesn't seem to be any validation of (f)s_maxsymlinklen being done. Unless I'm mistaken ufs_symlink could end up overwriting random memory if it contains a large bogus value. Does that all sound correct? If so would you like me to whip up a couple of patches to fix it? I'll respin the NUL-termination patch on top of those, if so. > /Evgeniy Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] ufs: ensure fast symlinks are NUL-terminated 2008-12-16 23:18 ` Duane Griffin @ 2008-12-27 12:22 ` Evgeniy Dushistov 0 siblings, 0 replies; 20+ messages in thread From: Evgeniy Dushistov @ 2008-12-27 12:22 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel On Tue, Dec 16, 2008 at 11:18:50PM +0000, Duane Griffin wrote: > On Tue, Dec 16, 2008 at 10:40:55PM +0300, Evgeniy Dushistov wrote: > > There is different types of ufs, one used 64 bit for "pointers to > > blocks", another 32 bit, > > so sizeof(UFS_I(inode)->i_u1.i_symlink)) > > is not right choice every time, > > in ufs2 it should be > > sizeof(UFS_I(inode)->i_u1.u2_i_data) which 2 times bigger, > > > > also there is hint for *BSD ufs > > > > fs/ufs/ufs_fs.h: > > __fs32 fs_maxsymlinklen;/* max length of an internal symlink */ > > > > which may be used if ufs type ufs1 or ufs2 > > Hmm, I see. However it looks like ufs1_read_inode and ufs2_read_inode > both copy the same, ((UFS_NDADDR + UFS_NINDIR) * 4), amount of inline > symlink data. They also both copy it to ufs_inode_info->i_u1.i_symlink > (not that that matters, I suppose). Perhaps I'm being obtuse, but it > looks like inline ufs2 symlinks between 60 and 120 characters long are > being truncated to 60 characters, no? > > There also doesn't seem to be any validation of (f)s_maxsymlinklen being > done. Unless I'm mistaken ufs_symlink could end up overwriting random > memory if it contains a large bogus value. > > Does that all sound correct? If so would you like me to whip up a couple > of patches to fix it? I'll respin the NUL-termination patch on top of > those, if so. > Yes, it looks like there is typo in ufs2 variant of copying symlink names. Typical value of superblock's maxsymlinklen field for ufs2 is 120. Patches to fix this are welcome. -- /Evgeniy ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-16 15:51 ` [PATCH] ext4: " Duane Griffin 2008-12-16 15:51 ` [PATCH] ufs: " Duane Griffin @ 2008-12-16 23:46 ` Andreas Dilger 2008-12-17 0:26 ` Duane Griffin 1 sibling, 1 reply; 20+ messages in thread From: Andreas Dilger @ 2008-12-16 23:46 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Al Viro, Andrew Morton, Theodore Ts'o, linux-ext4 On Dec 16, 2008 15:51 +0000, Duane Griffin wrote: > A number of filesystems were potentially triggering kernel bugs due to > corrupted symlink names on disk. This helper helps safely terminate the > names. > > +static inline void nd_terminate_link(void *name,unsigned len,unsigned maxlen) > +{ > + ((char *) name)[min(len, maxlen)] = '\0'; > +} > @@ -4200,9 +4201,11 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) > } else if (S_ISLNK(inode->i_mode)) { > + if (ext4_inode_is_fast_symlink(inode)) { > inode->i_op = &ext4_fast_symlink_inode_operations; > + nd_terminate_link(ei->i_data, inode->i_size, > + sizeof(ei->i_data)); > + } else { > inode->i_op = &ext4_symlink_inode_operations; > ext4_set_aops(inode); > } With sizeof(ei->i_data) = 15 * 4 = 60 bytes, this will set ei->i_data[60] as NUL, which is writing 1 byte beyond the end of the array. Note that in ext[234]_symlink() the check for fast symlinks is: l = strlen(symname)+1; if (l > sizeof (EXT3_I(inode)->i_data)) { inode->i_op = &ext3_symlink_inode_operations; } else { inode->i_op = &ext3_fast_symlink_inode_operations; inode->i_size = l-1; } so in fact the fast symlinks should always have space for a trailing NUL character, and "sizeof(ei->i_data) - 1" is the right maxlen to use for ext[234]. That might not be true for other filesystems, in which case you would need to add a "padding" field after the symlink name in memory to hold the trailing NUL. Cheers, Andreas -- Andreas Dilger Sr. Staff Engineer, Lustre Group Sun Microsystems of Canada, Inc. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-16 23:46 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Andreas Dilger @ 2008-12-17 0:26 ` Duane Griffin 2008-12-19 15:03 ` Duane Griffin 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-17 0:26 UTC (permalink / raw) To: Andreas Dilger Cc: linux-kernel, linux-fsdevel, Al Viro, Andrew Morton, Theodore Ts'o, linux-ext4 2008/12/16 Andreas Dilger <adilger@sun.com>: > On Dec 16, 2008 15:51 +0000, Duane Griffin wrote: >> A number of filesystems were potentially triggering kernel bugs due to >> corrupted symlink names on disk. This helper helps safely terminate the >> names. >> >> +static inline void nd_terminate_link(void *name,unsigned len,unsigned maxlen) >> +{ >> + ((char *) name)[min(len, maxlen)] = '\0'; >> +} > >> @@ -4200,9 +4201,11 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) >> } else if (S_ISLNK(inode->i_mode)) { >> + if (ext4_inode_is_fast_symlink(inode)) { >> inode->i_op = &ext4_fast_symlink_inode_operations; >> + nd_terminate_link(ei->i_data, inode->i_size, >> + sizeof(ei->i_data)); >> + } else { >> inode->i_op = &ext4_symlink_inode_operations; >> ext4_set_aops(inode); >> } > > With sizeof(ei->i_data) = 15 * 4 = 60 bytes, this will set ei->i_data[60] > as NUL, which is writing 1 byte beyond the end of the array. Argh, you are correct, of course. > Note that in ext[234]_symlink() the check for fast symlinks is: > > l = strlen(symname)+1; > if (l > sizeof (EXT3_I(inode)->i_data)) { > inode->i_op = &ext3_symlink_inode_operations; > } else { > inode->i_op = &ext3_fast_symlink_inode_operations; > inode->i_size = l-1; > } > > so in fact the fast symlinks should always have space for a trailing NUL > character, and "sizeof(ei->i_data) - 1" is the right maxlen to use for > ext[234]. > > That might not be true for other filesystems, in which case you would > need to add a "padding" field after the symlink name in memory to hold > the trailing NUL. It is true for sysv, too. The ufs code also, but that has other issues, anyway. The generic page symlink and freevxfs patches should be fine, though. Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-17 0:26 ` Duane Griffin @ 2008-12-19 15:03 ` Duane Griffin 2008-12-19 19:28 ` Andrew Morton 2008-12-19 19:43 ` Al Viro 0 siblings, 2 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-19 15:03 UTC (permalink / raw) To: Al Viro, Andrew Morton Cc: linux-kernel, linux-fsdevel, Andreas Dilger, Theodore Ts'o, linux-ext4 2008/12/17 Duane Griffin <duaneg@dghda.com>: > It is true for sysv, too. The ufs code also, but that has other > issues, anyway. The generic page symlink and freevxfs patches should > be fine, though. Before I fire off another set of patches, should I be sending out the complete set again or just the ones that need to be updated? Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-19 15:03 ` Duane Griffin @ 2008-12-19 19:28 ` Andrew Morton 2008-12-19 19:43 ` Al Viro 1 sibling, 0 replies; 20+ messages in thread From: Andrew Morton @ 2008-12-19 19:28 UTC (permalink / raw) To: Duane Griffin Cc: viro, linux-kernel, linux-fsdevel, adilger, tytso, linux-ext4 On Fri, 19 Dec 2008 15:03:52 +0000 "Duane Griffin" <duaneg@dghda.com> wrote: > 2008/12/17 Duane Griffin <duaneg@dghda.com>: > > It is true for sysv, too. The ufs code also, but that has other > > issues, anyway. The generic page symlink and freevxfs patches should > > be fine, though. > > Before I fire off another set of patches, should I be sending out the > complete set again or just the ones that need to be updated? > It's more reliable to resend everything. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-19 15:03 ` Duane Griffin 2008-12-19 19:28 ` Andrew Morton @ 2008-12-19 19:43 ` Al Viro 1 sibling, 0 replies; 20+ messages in thread From: Al Viro @ 2008-12-19 19:43 UTC (permalink / raw) To: Duane Griffin Cc: Andrew Morton, linux-kernel, linux-fsdevel, Andreas Dilger, Theodore Ts'o, linux-ext4 On Fri, Dec 19, 2008 at 03:03:52PM +0000, Duane Griffin wrote: > 2008/12/17 Duane Griffin <duaneg@dghda.com>: > > It is true for sysv, too. The ufs code also, but that has other > > issues, anyway. The generic page symlink and freevxfs patches should > > be fine, though. > > Before I fire off another set of patches, should I be sending out the > complete set again or just the ones that need to be updated? Complete set, please... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks 2008-12-16 15:51 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Duane Griffin 2008-12-16 15:51 ` [PATCH] vfs: ensure page symlinks are NUL-terminated Duane Griffin @ 2008-12-16 16:38 ` Al Viro 1 sibling, 0 replies; 20+ messages in thread From: Al Viro @ 2008-12-16 16:38 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Andrew Morton On Tue, Dec 16, 2008 at 03:51:53PM +0000, Duane Griffin wrote: > A number of filesystems were potentially triggering kernel bugs due to > corrupted symlink names on disk. This helper helps safely terminate the > names. s/unsigned/size_t/g and into the queue that goes... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/8, v3] ensure symlinks are NUL-terminated 2008-12-16 15:51 [PATCH 0/8, v3] ensure symlinks are NUL-terminated Duane Griffin 2008-12-16 15:51 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Duane Griffin @ 2008-12-16 17:03 ` Dave Kleikamp 2008-12-16 17:26 ` Duane Griffin 1 sibling, 1 reply; 20+ messages in thread From: Dave Kleikamp @ 2008-12-16 17:03 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Andrew Morton, Al Viro, Evgeniy Dushistov On Tue, 2008-12-16 at 15:51 +0000, Duane Griffin wrote: > These patches fix potential bugs associated with link target handling by > NUL-terminating names read from disk. > > This is version 3 of these patches. It fixes the bug, pointed out by Al Viro > and Evgeniy Dushistov, that i_size was not being validated. In order to > facilitate this it introduces a helper function for terminating the link name, > as suggested by Al. Duane, I see you left out jfs, so I'm assuming you're letting me handle that one. I've push my patch upstream, so it should show up in the -next tree. Shaggy > diffstat: > 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 +++++- > fs/ufs/inode.c | 8 ++++++-- > include/linux/namei.h | 5 +++++ > 8 files changed, 39 insertions(+), 12 deletions(-) -- David Kleikamp IBM Linux Technology Center ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/8, v3] ensure symlinks are NUL-terminated 2008-12-16 17:03 ` [PATCH 0/8, v3] ensure symlinks are NUL-terminated Dave Kleikamp @ 2008-12-16 17:26 ` Duane Griffin 0 siblings, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-16 17:26 UTC (permalink / raw) To: Dave Kleikamp Cc: linux-kernel, linux-fsdevel, Andrew Morton, Al Viro, Evgeniy Dushistov 2008/12/16 Dave Kleikamp <shaggy@linux.vnet.ibm.com>: > Duane, > I see you left out jfs, so I'm assuming you're letting me handle that > one. I've push my patch upstream, so it should show up in the -next > tree. That's right, since jfs doesn't need the helper and your patch was a full and complete fix. I should have clarified whether that was how you wanted it handled, sorry. Which reminds me, I need to follow up about the original befs and 9p patches which didn't make it to the list and haven't been ACKed or NAKed yet... > Shaggy Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2008-12-27 12:23 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-12-16 15:51 [PATCH 0/8, v3] ensure symlinks are NUL-terminated Duane Griffin 2008-12-16 15:51 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Duane Griffin 2008-12-16 15:51 ` [PATCH] vfs: ensure page symlinks are NUL-terminated Duane Griffin 2008-12-16 15:51 ` [PATCH] ext2: ensure fast " Duane Griffin 2008-12-16 15:51 ` [PATCH] ext3: " Duane Griffin 2008-12-16 15:51 ` [PATCH] ext4: " Duane Griffin 2008-12-16 15:51 ` [PATCH] ufs: " Duane Griffin 2008-12-16 15:51 ` [PATCH] sysv: " Duane Griffin 2008-12-16 15:52 ` [PATCH] freevxfs: " Duane Griffin 2008-12-16 19:40 ` [PATCH] ufs: " Evgeniy Dushistov 2008-12-16 23:18 ` Duane Griffin 2008-12-27 12:22 ` Evgeniy Dushistov 2008-12-16 23:46 ` [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Andreas Dilger 2008-12-17 0:26 ` Duane Griffin 2008-12-19 15:03 ` Duane Griffin 2008-12-19 19:28 ` Andrew Morton 2008-12-19 19:43 ` Al Viro 2008-12-16 16:38 ` Al Viro 2008-12-16 17:03 ` [PATCH 0/8, v3] ensure symlinks are NUL-terminated Dave Kleikamp 2008-12-16 17:26 ` Duane Griffin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.