* [PATCH 0/11] make link target handling more robust
@ 2008-12-11 19:16 Duane Griffin
[not found] ` <1229022995-9898-2-git-send-email-duaneg@dghda.com>
0 siblings, 1 reply; 20+ messages in thread
From: Duane Griffin @ 2008-12-11 19:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-fsdevel, Boaz Harrosh, Andrew Morton, Christoph Hellwig
These patches fix potential bugs associated with link target handling,
primarily by NULL-terminating names read from disk.
The ext2 and generic page_getlink changes have been tested by listing
corrupted links and creating and listing valid ones. Other patches have
been compile-tested only.
Cheers,
Duane.
^ permalink raw reply [flat|nested] 20+ messages in thread[parent not found: <1229022995-9898-2-git-send-email-duaneg@dghda.com>]
[parent not found: <1229022995-9898-3-git-send-email-duaneg@dghda.com>]
[parent not found: <1229022995-9898-4-git-send-email-duaneg@dghda.com>]
* [PATCH] ext2: ensure link targets are NULL-terminated [not found] ` <1229022995-9898-4-git-send-email-duaneg@dghda.com> @ 2008-12-11 19:16 ` Duane Griffin 2008-12-11 19:21 ` Matthew Wilcox ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-11 19:16 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Boaz Harrosh, Duane Griffin, linux-ext4 Ensure link targets are NULL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- fs/ext2/symlink.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/fs/ext2/symlink.c b/fs/ext2/symlink.c index 4e2426e..9b164ba 100644 --- a/fs/ext2/symlink.c +++ b/fs/ext2/symlink.c @@ -24,7 +24,9 @@ static void *ext2_follow_link(struct dentry *dentry, struct nameidata *nd) { struct ext2_inode_info *ei = EXT2_I(dentry->d_inode); - nd_set_link(nd, (char *)ei->i_data); + char *link = (char *) ei->i_data; + link[sizeof(ei->i_data) - 1] = '\0'; + nd_set_link(nd, link); return NULL; } -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] ext2: ensure link targets are NULL-terminated 2008-12-11 19:16 ` [PATCH] ext2: ensure link targets are NULL-terminated Duane Griffin @ 2008-12-11 19:21 ` Matthew Wilcox 2008-12-11 20:25 ` Duane Griffin 2008-12-11 22:23 ` [PATCH, v2] ext2: ensure link targets are NUL-terminated Duane Griffin [not found] ` <1229022995-9898-6-git-send-email-duaneg@dghda.com> 2 siblings, 1 reply; 20+ messages in thread From: Matthew Wilcox @ 2008-12-11 19:21 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, linux-ext4 On Thu, Dec 11, 2008 at 07:16:28PM +0000, Duane Griffin wrote: > Ensure link targets are NULL-terminated, even if corrupted on-disk. FWIW, that's spelled 'NUL' -- NULL is a pointer value, NUL is an ASCII code. > Signed-off-by: Duane Griffin <duaneg@dghda.com> > --- > fs/ext2/symlink.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/fs/ext2/symlink.c b/fs/ext2/symlink.c > index 4e2426e..9b164ba 100644 > --- a/fs/ext2/symlink.c > +++ b/fs/ext2/symlink.c > @@ -24,7 +24,9 @@ > static void *ext2_follow_link(struct dentry *dentry, struct nameidata *nd) > { > struct ext2_inode_info *ei = EXT2_I(dentry->d_inode); > - nd_set_link(nd, (char *)ei->i_data); > + char *link = (char *) ei->i_data; > + link[sizeof(ei->i_data) - 1] = '\0'; > + nd_set_link(nd, link); > return NULL; > } > > -- > 1.6.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] ext2: ensure link targets are NULL-terminated 2008-12-11 19:21 ` Matthew Wilcox @ 2008-12-11 20:25 ` Duane Griffin 0 siblings, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-11 20:25 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, linux-ext4 2008/12/11 Matthew Wilcox <matthew@wil.cx>: > On Thu, Dec 11, 2008 at 07:16:28PM +0000, Duane Griffin wrote: >> Ensure link targets are NULL-terminated, even if corrupted on-disk. > > FWIW, that's spelled 'NUL' -- NULL is a pointer value, NUL is an ASCII > code. Good point, thanks. I'll try to remember for the future! 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
* [PATCH, v2] ext2: ensure link targets are NUL-terminated 2008-12-11 19:16 ` [PATCH] ext2: ensure link targets are NULL-terminated Duane Griffin 2008-12-11 19:21 ` Matthew Wilcox @ 2008-12-11 22:23 ` Duane Griffin [not found] ` <1229022995-9898-6-git-send-email-duaneg@dghda.com> 2 siblings, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-11 22:23 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, linux-ext4 Ensure link targets are NUL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- V2: terminate when the link is read instead of every time it is followed, as suggested by Dave Kleikamp. diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index 7658b33..9bacf35 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -1286,9 +1286,10 @@ 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 { + ((char *) ei->i_data)[inode->i_size] = '\0'; + } else { inode->i_op = &ext2_symlink_inode_operations; if (test_opt(inode->i_sb, NOBH)) inode->i_mapping->a_ops = &ext2_nobh_aops; -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply related [flat|nested] 20+ messages in thread
[parent not found: <1229022995-9898-6-git-send-email-duaneg@dghda.com>]
* [PATCH, v2] ext3: ensure link targets are NULL-terminated [not found] ` <1229022995-9898-6-git-send-email-duaneg@dghda.com> @ 2008-12-11 22:26 ` Duane Griffin 2008-12-12 3:32 ` Andrew Morton [not found] ` <1229022995-9898-7-git-send-email-duaneg@dghda.com> 1 sibling, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-11 22:26 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, Stephen, Tweedie, sct, Andrew, Morton, akpm, adilger, linux-ext4 Ensure link targets are NUL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- V2: terminate when the link is read instead of every time it is followed, as suggested by Dave Kleikamp. diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c index f8424ad..c168781 100644 --- a/fs/ext3/inode.c +++ b/fs/ext3/inode.c @@ -2817,9 +2817,10 @@ 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 { + ((char *) ei->i_data)[inode->i_size] = '\0'; + } else { inode->i_op = &ext3_symlink_inode_operations; ext3_set_aops(inode); } -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH, v2] ext3: ensure link targets are NULL-terminated 2008-12-11 22:26 ` [PATCH, v2] ext3: ensure link targets are NULL-terminated Duane Griffin @ 2008-12-12 3:32 ` Andrew Morton 2008-12-12 9:40 ` Duane Griffin 0 siblings, 1 reply; 20+ messages in thread From: Andrew Morton @ 2008-12-12 3:32 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, Stephen, Tweedie, sct, Andrew, Morton, adilger, linux-ext4 On Thu, 11 Dec 2008 22:26:05 +0000 "Duane Griffin" <duaneg@dghda.com> wrote: > Ensure link targets are NUL-terminated, even if corrupted on-disk. > > Signed-off-by: Duane Griffin <duaneg@dghda.com> > --- > > V2: terminate when the link is read instead of every time it is > followed, as suggested by Dave Kleikamp. > > diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c > index f8424ad..c168781 100644 > --- a/fs/ext3/inode.c > +++ b/fs/ext3/inode.c > @@ -2817,9 +2817,10 @@ 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 { > + ((char *) ei->i_data)[inode->i_size] = '\0'; > + } else { > inode->i_op = &ext3_symlink_inode_operations; > ext3_set_aops(inode); > } Really? The ext2 on-disk format requires that the fast symlink be null-terminated on disk? Even though the length is already in i_size? It seems that's true. How un-ext2-like. ext2 and ext4 need the same fix, yes? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH, v2] ext3: ensure link targets are NULL-terminated 2008-12-12 3:32 ` Andrew Morton @ 2008-12-12 9:40 ` Duane Griffin 2008-12-12 10:19 ` Duane Griffin 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-12 9:40 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, sct, adilger, linux-ext4 2008/12/12 Andrew Morton <akpm@linux-foundation.org>: > Really? The ext2 on-disk format requires that the fast symlink be > null-terminated on disk? Even though the length is already in i_size? > > It seems that's true. How un-ext2-like. > > ext2 and ext4 need the same fix, yes? Yes. I've sent them out already, but thanks to a monumental cock-up with the CCs they may not have made it to the list. I'll check and resend to real addresses if necessary. 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, v2] ext3: ensure link targets are NULL-terminated 2008-12-12 9:40 ` Duane Griffin @ 2008-12-12 10:19 ` Duane Griffin 2008-12-16 0:09 ` Andrew Morton 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-12 10:19 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, sct, adilger, linux-ext4 2008/12/12 Duane Griffin <duaneg@dghda.com>: > 2008/12/12 Andrew Morton <akpm@linux-foundation.org>: >> Really? The ext2 on-disk format requires that the fast symlink be >> null-terminated on disk? Even though the length is already in i_size? >> >> It seems that's true. How un-ext2-like. >> >> ext2 and ext4 need the same fix, yes? > > Yes. I've sent them out already, but thanks to a monumental cock-up > with the CCs they may not have made it to the list. I'll check and > resend to real addresses if necessary. Seems they did make it: http://marc.info/?l=linux-kernel&m=122903437006575&w=2 http://marc.info/?l=linux-kernel&m=122903451306859&w=2 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, v2] ext3: ensure link targets are NULL-terminated 2008-12-12 10:19 ` Duane Griffin @ 2008-12-16 0:09 ` Andrew Morton 0 siblings, 0 replies; 20+ messages in thread From: Andrew Morton @ 2008-12-16 0:09 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, bharrosh, sct, adilger, linux-ext4 On Fri, 12 Dec 2008 10:19:20 +0000 "Duane Griffin" <duaneg@dghda.com> wrote: > 2008/12/12 Duane Griffin <duaneg@dghda.com>: > > 2008/12/12 Andrew Morton <akpm@linux-foundation.org>: > >> Really? The ext2 on-disk format requires that the fast symlink be > >> null-terminated on disk? Even though the length is already in i_size? > >> > >> It seems that's true. How un-ext2-like. > >> > >> ext2 and ext4 need the same fix, yes? > > > > Yes. I've sent them out already, but thanks to a monumental cock-up > > with the CCs they may not have made it to the list. I'll check and > > resend to real addresses if necessary. > > Seems they did make it: > http://marc.info/?l=linux-kernel&m=122903437006575&w=2 > http://marc.info/?l=linux-kernel&m=122903451306859&w=2 > OK, thanks, it seems I was sneakily not cc'ed ;) As Al points out, the code which you implemented is still vulnerable to on-disk corruption: bad values of i_size will cause the kernel to write a zero byte to any address within the entire CPU address range. ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <1229022995-9898-7-git-send-email-duaneg@dghda.com>]
* [PATCH, v2] ext4: ensure link targets are NUL-terminated [not found] ` <1229022995-9898-7-git-send-email-duaneg@dghda.com> @ 2008-12-11 22:27 ` Duane Griffin [not found] ` <1229022995-9898-8-git-send-email-duaneg@dghda.com> 1 sibling, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-11 22:27 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, tytso, adilger, linux-ext4 Ensure link targets are NUL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- V2: terminate when the link is read instead of every time it is followed, as suggested by Dave Kleikamp. diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index ca88060..e70122e 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4200,9 +4200,10 @@ 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 { + ((char *) ei->i_data)[inode->i_size] = '\0'; + } else { inode->i_op = &ext4_symlink_inode_operations; ext4_set_aops(inode); } -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply related [flat|nested] 20+ messages in thread
[parent not found: <1229022995-9898-8-git-send-email-duaneg@dghda.com>]
[parent not found: <1229022995-9898-9-git-send-email-duaneg@dghda.com>]
* Re: [PATCH] JFS: ensure link targets are NULL-terminated [not found] ` <1229022995-9898-9-git-send-email-duaneg@dghda.com> @ 2008-12-11 20:06 ` Dave Kleikamp 2008-12-11 20:34 ` Duane Griffin [not found] ` <1229022995-9898-10-git-send-email-duaneg@dghda.com> 1 sibling, 1 reply; 20+ messages in thread From: Dave Kleikamp @ 2008-12-11 20:06 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, Dave.Kleikamp.shaggy, jfs-discussion On Thu, 2008-12-11 at 19:16 +0000, Duane Griffin wrote: > Ensure link targets are NULL-terminated, even if corrupted on-disk. > > Signed-off-by: Duane Griffin <duaneg@dghda.com> > --- > fs/jfs/symlink.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/fs/jfs/symlink.c b/fs/jfs/symlink.c > index 4af1a05..f5f1b5d 100644 > --- a/fs/jfs/symlink.c > +++ b/fs/jfs/symlink.c > @@ -18,6 +18,7 @@ > > #include <linux/fs.h> > #include <linux/namei.h> > +#include "jfs_filsys.h" > #include "jfs_incore.h" > #include "jfs_inode.h" > #include "jfs_xattr.h" > @@ -25,6 +26,7 @@ > static void *jfs_follow_link(struct dentry *dentry, struct nameidata *nd) > { > char *s = JFS_IP(dentry->d_inode)->i_inline; > + s[IDATASIZE - 1] = '\0'; > nd_set_link(nd, s); > return NULL; > } Thanks. I came up with an alternate patch that I like slightly better. This one null-terminates the string when the inode is read rather than every time the link is followed, and at the proper place (i_size). It's not a big deal, since the symlink will be corrupted either way. What do you think of this patch? Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com> diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c index 2103397..b00ee9f 100644 --- a/fs/jfs/inode.c +++ b/fs/jfs/inode.c @@ -59,8 +59,14 @@ struct inode *jfs_iget(struct super_block *sb, unsigned long ino) if (inode->i_size >= IDATASIZE) { inode->i_op = &page_symlink_inode_operations; inode->i_mapping->a_ops = &jfs_aops; - } else + } else { inode->i_op = &jfs_symlink_inode_operations; + /* + * The inline data should be null-terminated, but + * don't let on-disk corruption crash the kernel + */ + JFS_IP(inode)->i_inline[inode->i_size] = '\0'; + } } else { inode->i_op = &jfs_file_inode_operations; init_special_inode(inode, inode->i_mode, inode->i_rdev); -- David Kleikamp IBM Linux Technology Center ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] JFS: ensure link targets are NULL-terminated 2008-12-11 20:06 ` [PATCH] JFS: ensure link targets are NULL-terminated Dave Kleikamp @ 2008-12-11 20:34 ` Duane Griffin 0 siblings, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-11 20:34 UTC (permalink / raw) To: Dave Kleikamp Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, Dave.Kleikamp.shaggy, jfs-discussion 2008/12/11 Dave Kleikamp <shaggy@linux.vnet.ibm.com>: > Thanks. > > I came up with an alternate patch that I like slightly better. This one > null-terminates the string when the inode is read rather than every time > the link is followed, and at the proper place (i_size). It's not a big > deal, since the symlink will be corrupted either way. What do you think > of this patch? > > Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com> > > diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c > index 2103397..b00ee9f 100644 > --- a/fs/jfs/inode.c > +++ b/fs/jfs/inode.c > @@ -59,8 +59,14 @@ struct inode *jfs_iget(struct super_block *sb, unsigned long ino) > if (inode->i_size >= IDATASIZE) { > inode->i_op = &page_symlink_inode_operations; > inode->i_mapping->a_ops = &jfs_aops; > - } else > + } else { > inode->i_op = &jfs_symlink_inode_operations; > + /* > + * The inline data should be null-terminated, but > + * don't let on-disk corruption crash the kernel > + */ > + JFS_IP(inode)->i_inline[inode->i_size] = '\0'; > + } > } else { > inode->i_op = &jfs_file_inode_operations; > init_special_inode(inode, inode->i_mode, inode->i_rdev); Yes, I prefer your version too :) I'll rework the other patches similarly, where appropriate. Thanks! 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
[parent not found: <1229022995-9898-10-git-send-email-duaneg@dghda.com>]
[parent not found: <1229022995-9898-11-git-send-email-duaneg@dghda.com>]
* [PATCH, v2] sysv: ensure link targets are NUL-terminated [not found] ` <1229022995-9898-11-git-send-email-duaneg@dghda.com> @ 2008-12-11 22:35 ` Duane Griffin 2008-12-15 10:12 ` Al Viro [not found] ` <1229022995-9898-12-git-send-email-duaneg@dghda.com> 1 sibling, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-11 22:35 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, hch Ensure link targets are NUL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- V2: terminate when the link is read instead of every time it is followed, as suggested by Dave Kleikamp. diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c index df0d435..1511228 100644 --- a/fs/sysv/inode.c +++ b/fs/sysv/inode.c @@ -163,8 +163,10 @@ 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; + ((char *) SYSV_I(inode)->i_data)[inode->i_size] = '\0'; + } } else init_special_inode(inode, inode->i_mode, rdev); } -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH, v2] sysv: ensure link targets are NUL-terminated 2008-12-11 22:35 ` [PATCH, v2] sysv: ensure link targets are NUL-terminated Duane Griffin @ 2008-12-15 10:12 ` Al Viro 0 siblings, 0 replies; 20+ messages in thread From: Al Viro @ 2008-12-15 10:12 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, hch On Thu, Dec 11, 2008 at 10:35:32PM +0000, Duane Griffin wrote: > Ensure link targets are NUL-terminated, even if corrupted on-disk. > > Signed-off-by: Duane Griffin <duaneg@dghda.com> > --- > > V2: terminate when the link is read instead of every time it is > followed, as suggested by Dave Kleikamp. > > diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c > index df0d435..1511228 100644 > --- a/fs/sysv/inode.c > +++ b/fs/sysv/inode.c > @@ -163,8 +163,10 @@ 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; > + ((char *) SYSV_I(inode)->i_data)[inode->i_size] = '\0'; If you do it that way, you want to verify that i_size is bounded. Better yet, add a helper for that (taking void *, len, max_len) ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <1229022995-9898-12-git-send-email-duaneg@dghda.com>]
* [PATCH, v2] ufs: ensure link targets are NUL-terminated [not found] ` <1229022995-9898-12-git-send-email-duaneg@dghda.com> @ 2008-12-11 22:36 ` Duane Griffin 2008-12-14 21:12 ` Evgeniy Dushistov 0 siblings, 1 reply; 20+ messages in thread From: Duane Griffin @ 2008-12-11 22:36 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, dushistov Ensure link targets are NUL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- V2: terminate when the link is read instead of every time it is followed, as suggested by Dave Kleikamp. diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c index 39f8778..a22f64d 100644 --- a/fs/ufs/inode.c +++ b/fs/ufs/inode.c @@ -606,9 +606,11 @@ 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) { + char *link = UFS_I(inode)->i_u1.i_symlink; inode->i_op = &ufs_fast_symlink_inode_operations; - else { + link[inode->i_size] = '\0'; + } else { inode->i_op = &page_symlink_inode_operations; inode->i_mapping->a_ops = &ufs_aops; } -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH, v2] ufs: ensure link targets are NUL-terminated 2008-12-11 22:36 ` [PATCH, v2] ufs: " Duane Griffin @ 2008-12-14 21:12 ` Evgeniy Dushistov 0 siblings, 0 replies; 20+ messages in thread From: Evgeniy Dushistov @ 2008-12-14 21:12 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh On Thu, Dec 11, 2008 at 10:36:08PM +0000, Duane Griffin wrote: > Ensure link targets are NUL-terminated, even if corrupted on-disk. > > Signed-off-by: Duane Griffin <duaneg@dghda.com> > --- > > V2: terminate when the link is read instead of every time it is > followed, as suggested by Dave Kleikamp. > > diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c > index 39f8778..a22f64d 100644 > --- a/fs/ufs/inode.c > +++ b/fs/ufs/inode.c > @@ -606,9 +606,11 @@ 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) { > + char *link = UFS_I(inode)->i_u1.i_symlink; > inode->i_op = &ufs_fast_symlink_inode_operations; > - else { > + link[inode->i_size] = '\0'; > + } else { > inode->i_op = &page_symlink_inode_operations; > inode->i_mapping->a_ops = &ufs_aops; > } if we talk about corrupted file system, may be also ensure that inode->i_size <= sizeof(i_u1)-1 before write '\0' to it. -- /Evgeniy ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH, v2] freevxfs: ensure link targets are NUL-terminated [not found] ` <1229022995-9898-8-git-send-email-duaneg@dghda.com> [not found] ` <1229022995-9898-9-git-send-email-duaneg@dghda.com> @ 2008-12-11 22:37 ` Duane Griffin 1 sibling, 0 replies; 20+ messages in thread From: Duane Griffin @ 2008-12-11 22:37 UTC (permalink / raw) To: Duane Griffin; +Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, hch Ensure link targets are NUL-terminated, even if corrupted on-disk. Signed-off-by: Duane Griffin <duaneg@dghda.com> --- V2: terminate when the link is read instead of every time it is followed, as suggested by Dave Kleikamp. 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)); -- "I never could learn to drink that blood and call it wine" - Bob Dylan ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] eCryptfs: check readlink result was not an error before using it [not found] ` <1229022995-9898-3-git-send-email-duaneg@dghda.com> [not found] ` <1229022995-9898-4-git-send-email-duaneg@dghda.com> @ 2008-12-11 19:32 ` Michael Halcrow 2008-12-12 1:48 ` Tim Gardner 1 sibling, 1 reply; 20+ messages in thread From: Michael Halcrow @ 2008-12-11 19:32 UTC (permalink / raw) To: Duane Griffin Cc: linux-kernel, linux-fsdevel, Boaz Harrosh, Mike.Halcrow.mhalcrow, Phillip.Hellewell.phillip, ecryptfs-devel On Thu, Dec 11, 2008 at 07:16:26PM +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. > > Signed-off-by: Duane Griffin <duaneg@dghda.com> Acked-by: Michael Halcrow <mhalcrow@us.ibm.com> > --- > 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; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] eCryptfs: check readlink result was not an error before using it 2008-12-11 19:32 ` [PATCH] eCryptfs: check readlink result was not an error before using it Michael Halcrow @ 2008-12-12 1:48 ` Tim Gardner 0 siblings, 0 replies; 20+ messages in thread From: Tim Gardner @ 2008-12-12 1:48 UTC (permalink / raw) To: Michael Halcrow Cc: Duane Griffin, linux-kernel, linux-fsdevel, Boaz Harrosh, Mike.Halcrow.mhalcrow, Phillip.Hellewell.phillip, ecryptfs-devel, stable Michael Halcrow wrote: > On Thu, Dec 11, 2008 at 07:16:26PM +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. >> >> Signed-off-by: Duane Griffin <duaneg@dghda.com> > > Acked-by: Michael Halcrow <mhalcrow@us.ibm.com> > >> --- >> 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; > -- > 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/ > Please add 'Cc: stable@kernel.org' in the commit message. This looks like a good candidate. rtg -- Tim Gardner tim.gardner@canonical.com ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2008-12-16 0:09 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-11 19:16 [PATCH 0/11] make link target handling more robust Duane Griffin
[not found] ` <1229022995-9898-2-git-send-email-duaneg@dghda.com>
[not found] ` <1229022995-9898-3-git-send-email-duaneg@dghda.com>
[not found] ` <1229022995-9898-4-git-send-email-duaneg@dghda.com>
2008-12-11 19:16 ` [PATCH] ext2: ensure link targets are NULL-terminated Duane Griffin
2008-12-11 19:21 ` Matthew Wilcox
2008-12-11 20:25 ` Duane Griffin
2008-12-11 22:23 ` [PATCH, v2] ext2: ensure link targets are NUL-terminated Duane Griffin
[not found] ` <1229022995-9898-6-git-send-email-duaneg@dghda.com>
2008-12-11 22:26 ` [PATCH, v2] ext3: ensure link targets are NULL-terminated Duane Griffin
2008-12-12 3:32 ` Andrew Morton
2008-12-12 9:40 ` Duane Griffin
2008-12-12 10:19 ` Duane Griffin
2008-12-16 0:09 ` Andrew Morton
[not found] ` <1229022995-9898-7-git-send-email-duaneg@dghda.com>
2008-12-11 22:27 ` [PATCH, v2] ext4: ensure link targets are NUL-terminated Duane Griffin
[not found] ` <1229022995-9898-8-git-send-email-duaneg@dghda.com>
[not found] ` <1229022995-9898-9-git-send-email-duaneg@dghda.com>
2008-12-11 20:06 ` [PATCH] JFS: ensure link targets are NULL-terminated Dave Kleikamp
2008-12-11 20:34 ` Duane Griffin
[not found] ` <1229022995-9898-10-git-send-email-duaneg@dghda.com>
[not found] ` <1229022995-9898-11-git-send-email-duaneg@dghda.com>
2008-12-11 22:35 ` [PATCH, v2] sysv: ensure link targets are NUL-terminated Duane Griffin
2008-12-15 10:12 ` Al Viro
[not found] ` <1229022995-9898-12-git-send-email-duaneg@dghda.com>
2008-12-11 22:36 ` [PATCH, v2] ufs: " Duane Griffin
2008-12-14 21:12 ` Evgeniy Dushistov
2008-12-11 22:37 ` [PATCH, v2] freevxfs: " Duane Griffin
2008-12-11 19:32 ` [PATCH] eCryptfs: check readlink result was not an error before using it Michael Halcrow
2008-12-12 1:48 ` Tim Gardner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).