| Message ID | 1229442720-31056-6-git-send-email-duaneg@dghda.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(-)