[PATCH] ext2: ensure link targets are NULL-terminated

[PATCH] ext2: ensure link targets are NULL-terminated

[Duane Griffin]

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

Re: [PATCH] ext2: ensure link targets are NULL-terminated

[Matthew Wilcox]

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

Re: [PATCH] ext2: ensure link targets are NULL-terminated

[Duane Griffin]

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

[PATCH, v2] ext2: ensure link targets are NUL-terminated

[Duane Griffin]

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

[PATCH, v2] ext3: ensure link targets are NULL-terminated

[Duane Griffin]

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

[PATCH, v2] ext4: ensure link targets are NUL-terminated

[Duane Griffin]

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

Re: [PATCH, v2] ext3: ensure link targets are NULL-terminated

[Andrew Morton]

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?

Re: [PATCH, v2] ext3: ensure link targets are NULL-terminated

[Duane Griffin]

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

Re: [PATCH, v2] ext3: ensure link targets are NULL-terminated

[Duane Griffin]

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

Re: [PATCH, v2] ext3: ensure link targets are NULL-terminated

[Andrew Morton]

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.