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

[PATCH] 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 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

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

[Al Viro]

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...

[PATCH, v4] 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 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>
---

Version 4, this time with size_t instead of unsigned. Thanks Al!

 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, size_t len, size_t maxlen)
+{
+	((char *) name)[min(len, maxlen)] = '\0';
+}
+
 #endif /* _LINUX_NAMEI_H */
-- 
1.6.0.4

[PATCH] 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 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, size_t len, size_t maxlen)
+{
+	((char *) name)[min(len, maxlen)] = '\0';
+}
+
 #endif /* _LINUX_NAMEI_H */
-- 
1.6.0.4

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

[Andreas Dilger]

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.

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

[Duane Griffin]

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

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

[Duane Griffin]

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

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

[Andrew Morton]

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.

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

[Al Viro]

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...