[PATCH 1/4] ufs: validate maximum fast symlink size from superblock

[PATCH 1/4] ufs: validate maximum fast symlink size from superblock

[Duane Griffin]

The maximum fast symlink size is set in the superblock of certain types
of UFS filesystem. Before using it we need to check that it isn't longer
than the available space we have in the inode.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
---
 fs/ufs/super.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index e65212d..a959cdc 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -636,6 +636,7 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 	unsigned block_size, super_block_size;
 	unsigned flags;
 	unsigned super_block_offset;
+	unsigned maxsymlen;
 	int ret = -EINVAL;
 
 	uspi = NULL;
@@ -1069,6 +1070,16 @@ magic_found:
 		uspi->s_maxsymlinklen =
 		    fs32_to_cpu(sb, usb3->fs_un2.fs_44.fs_maxsymlinklen);
 
+	if (uspi->fs_magic == UFS2_MAGIC)
+		maxsymlen = 2 * 4 * (UFS_NDADDR + UFS_NINDIR);
+	else
+		maxsymlen = 4 * (UFS_NDADDR + UFS_NINDIR);
+	if (uspi->s_maxsymlinklen > maxsymlen) {
+		printk(KERN_WARNING "ufs_read_super: excessive maximum fast "
+		       "symlink size (%u)\n", uspi->s_maxsymlinklen);
+		uspi->s_maxsymlinklen = maxsymlen;
+	}
+
 	inode = ufs_iget(sb, UFS_ROOTINO);
 	if (IS_ERR(inode)) {
 		ret = PTR_ERR(inode);
-- 
1.6.0.6

[PATCH 2/4] ufs: don't truncate longer ufs2 fast symlinks

[Duane Griffin]

ufs2 fast symlinks can be twice as long as ufs ones, however the code
was using the ufs size in various places. Fix that so ufs2 symlinks over
60 characters aren't truncated.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
---
 fs/ufs/inode.c |    4 ++--
 fs/ufs/ufs.h   |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 39f8778..c1ea916 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -708,7 +708,7 @@ static int ufs2_read_inode(struct inode *inode, struct ufs2_inode *ufs2_inode)
 			ufsi->i_u1.u2_i_data[i] =
 				ufs2_inode->ui_u2.ui_addr.ui_db[i];
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
+		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
 			ufsi->i_u1.i_symlink[i] = ufs2_inode->ui_u2.ui_symlink[i];
 	}
 	return 0;
@@ -853,7 +853,7 @@ static void ufs2_update_inode(struct inode *inode, struct ufs2_inode *ufs_inode)
 		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR); i++)
 			ufs_inode->ui_u2.ui_addr.ui_db[i] = ufsi->i_u1.u2_i_data[i];
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
+		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
 			ufs_inode->ui_u2.ui_symlink[i] = ufsi->i_u1.i_symlink[i];
  	}
 
diff --git a/fs/ufs/ufs.h b/fs/ufs/ufs.h
index 11c0351..69b3427 100644
--- a/fs/ufs/ufs.h
+++ b/fs/ufs/ufs.h
@@ -23,7 +23,7 @@ struct ufs_sb_info {
 struct ufs_inode_info {
 	union {
 		__fs32	i_data[15];
-		__u8	i_symlink[4*15];
+		__u8	i_symlink[2 * 4 * 15];
 		__fs64	u2_i_data[15];
 	} i_u1;
 	__u32	i_flags;
-- 
1.6.0.6

[PATCH 3/4] ufs: ensure fast symlinks are NUL-terminated

[Duane Griffin]

Ensure fast symlink targets are NUL-terminated, even if corrupted
on-disk.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
---
 fs/ufs/inode.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index c1ea916..c2edfb6 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -658,8 +658,9 @@ static int ufs1_read_inode(struct inode *inode, struct ufs_inode *ufs_inode)
 		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR); i++)
 			ufsi->i_u1.i_data[i] = ufs_inode->ui_u2.ui_addr.ui_db[i];
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
+		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 - 1; i++)
 			ufsi->i_u1.i_symlink[i] = ufs_inode->ui_u2.ui_symlink[i];
+		ufsi->i_u1.i_symlink[(UFS_NDADDR + UFS_NINDIR) * 4 - 1] = 0;
 	}
 	return 0;
 }
@@ -708,8 +709,9 @@ static int ufs2_read_inode(struct inode *inode, struct ufs2_inode *ufs2_inode)
 			ufsi->i_u1.u2_i_data[i] =
 				ufs2_inode->ui_u2.ui_addr.ui_db[i];
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
+		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2 - 1; i++)
 			ufsi->i_u1.i_symlink[i] = ufs2_inode->ui_u2.ui_symlink[i];
+		ufsi->i_u1.i_symlink[(UFS_NDADDR + UFS_NINDIR) * 4 * 2 - 1] = 0;
 	}
 	return 0;
 }
-- 
1.6.0.6

[PATCH 4/4] ufs: copy symlink data into the correct union member

[Duane Griffin]

Copy symlink data into the union member it is accessed through. Although
this shouldn't make a difference to behaviour it makes the code easier
to follow and grep through. It may also prevent problems if the
struct/union definitions change in the future.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
---
 fs/ufs/namei.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c
index e3a9b1f..3884730 100644
--- a/fs/ufs/namei.c
+++ b/fs/ufs/namei.c
@@ -147,7 +147,7 @@ static int ufs_symlink (struct inode * dir, struct dentry * dentry,
 	} else {
 		/* fast symlink */
 		inode->i_op = &ufs_fast_symlink_inode_operations;
-		memcpy((char*)&UFS_I(inode)->i_u1.i_data,symname,l);
+		memcpy((char *) &UFS_I(inode)->i_u1.i_symlink, symname, l);
 		inode->i_size = l-1;
 	}
 	mark_inode_dirty(inode);
-- 
1.6.0.6

Re: [PATCH 2/4] ufs: don't truncate longer ufs2 fast symlinks

[Al Viro]

On Wed, Dec 31, 2008 at 04:50:06PM +0000, Duane Griffin wrote:
>  	} else {
> -		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
> +		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
>  			ufsi->i_u1.i_symlink[i] = ufs2_inode->ui_u2.ui_symlink[i];

a) that probably ought to be a memcpy()
b) max len is superblock parameter and you've even validated it.  Why don't
you use it?

> -		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
> +		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
>  			ufs_inode->ui_u2.ui_symlink[i] = ufsi->i_u1.i_symlink[i];

Ditto.

Re: [PATCH 4/4] ufs: copy symlink data into the correct union member

[Al Viro]

On Wed, Dec 31, 2008 at 04:50:08PM +0000, Duane Griffin wrote:
> Copy symlink data into the union member it is accessed through. Although
> this shouldn't make a difference to behaviour it makes the code easier
> to follow and grep through. It may also prevent problems if the
> struct/union definitions change in the future.
> 
> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> ---
>  fs/ufs/namei.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c
> index e3a9b1f..3884730 100644
> --- a/fs/ufs/namei.c
> +++ b/fs/ufs/namei.c
> @@ -147,7 +147,7 @@ static int ufs_symlink (struct inode * dir, struct dentry * dentry,
>  	} else {
>  		/* fast symlink */
>  		inode->i_op = &ufs_fast_symlink_inode_operations;
> -		memcpy((char*)&UFS_I(inode)->i_u1.i_data,symname,l);
> +		memcpy((char *) &UFS_I(inode)->i_u1.i_symlink, symname, l);

Just what are these cast and & doing there?  i_symlink is an array of u8,
for fsck sake; taking its address is a pointless obfuscation.  And memcpy()
is just as fine with u8 * as it is with char *...

Re: [PATCH 2/4] ufs: don't truncate longer ufs2 fast symlinks

[Duane Griffin]

On Thu, Jan 01, 2009 at 02:32:32AM +0000, Al Viro wrote:
> On Wed, Dec 31, 2008 at 04:50:06PM +0000, Duane Griffin wrote:
> >  	} else {
> > -		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
> > +		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
> >  			ufsi->i_u1.i_symlink[i] = ufs2_inode->ui_u2.ui_symlink[i];
> 
> a) that probably ought to be a memcpy()

OK, see below. I've changed the other close-by loops as well, for
consistency. Note however that they were copying an entire structure
(consisting of two arrays of the same type) whilst seeming to copy only
the first member. So the patch looks like it is changing behaviour, but
actually should not. Please let me know if you prefer this; I'll resend
the series if so.

> b) max len is superblock parameter and you've even validated it.  Why don't
> you use it?

It is only validated to an upper bound. If it is incorrectly set to
zero, say, this way at least the link names will show up. True, we copy
a bit of extra data around, but since it isn't exactly a fast path I
figured it was worth it for the extra reliability.

> 
> > -		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
> > +		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4 * 2; i++)
> >  			ufs_inode->ui_u2.ui_symlink[i] = ufsi->i_u1.i_symlink[i];
> 
> Ditto.

---

diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 39f8778..ac8b324 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -622,7 +622,6 @@ static int ufs1_read_inode(struct inode *inode, struct ufs_inode *ufs_inode)
 	struct ufs_inode_info *ufsi = UFS_I(inode);
 	struct super_block *sb = inode->i_sb;
 	mode_t mode;
-	unsigned i;
 
 	/*
 	 * Copy data to the in-core inode.
@@ -655,11 +654,11 @@ static int ufs1_read_inode(struct inode *inode, struct ufs_inode *ufs_inode)
 
 	
 	if (S_ISCHR(mode) || S_ISBLK(mode) || inode->i_blocks) {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR); i++)
-			ufsi->i_u1.i_data[i] = ufs_inode->ui_u2.ui_addr.ui_db[i];
+		memcpy(ufsi->i_u1.i_data, &ufs_inode->ui_u2.ui_addr,
+		       sizeof(ufs_inode->ui_u2.ui_addr));
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
-			ufsi->i_u1.i_symlink[i] = ufs_inode->ui_u2.ui_symlink[i];
+		memcpy(ufsi->i_u1.i_symlink, ufs_inode->ui_u2.ui_symlink,
+		       sizeof(ufs_inode->ui_u2.ui_symlink));
 	}
 	return 0;
 }
@@ -669,7 +668,6 @@ static int ufs2_read_inode(struct inode *inode, struct ufs2_inode *ufs2_inode)
 	struct ufs_inode_info *ufsi = UFS_I(inode);
 	struct super_block *sb = inode->i_sb;
 	mode_t mode;
-	unsigned i;
 
 	UFSD("Reading ufs2 inode, ino %lu\n", inode->i_ino);
 	/*
@@ -704,12 +702,11 @@ static int ufs2_read_inode(struct inode *inode, struct ufs2_inode *ufs2_inode)
 	*/
 
 	if (S_ISCHR(mode) || S_ISBLK(mode) || inode->i_blocks) {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR); i++)
-			ufsi->i_u1.u2_i_data[i] =
-				ufs2_inode->ui_u2.ui_addr.ui_db[i];
+		memcpy(ufsi->i_u1.u2_i_data, &ufs2_inode->ui_u2.ui_addr,
+		       sizeof(ufs2_inode->ui_u2.ui_addr));
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
-			ufsi->i_u1.i_symlink[i] = ufs2_inode->ui_u2.ui_symlink[i];
+		memcpy(ufsi->i_u1.i_symlink, ufs2_inode->ui_u2.ui_symlink,
+		       sizeof(ufs2_inode->ui_u2.ui_symlink));
 	}
 	return 0;
 }
@@ -781,7 +778,6 @@ static void ufs1_update_inode(struct inode *inode, struct ufs_inode *ufs_inode)
 {
 	struct super_block *sb = inode->i_sb;
  	struct ufs_inode_info *ufsi = UFS_I(inode);
- 	unsigned i;
 
 	ufs_inode->ui_mode = cpu_to_fs16(sb, inode->i_mode);
 	ufs_inode->ui_nlink = cpu_to_fs16(sb, inode->i_nlink);
@@ -809,12 +805,12 @@ static void ufs1_update_inode(struct inode *inode, struct ufs_inode *ufs_inode)
 		/* ufs_inode->ui_u2.ui_addr.ui_db[0] = cpu_to_fs32(sb, inode->i_rdev); */
 		ufs_inode->ui_u2.ui_addr.ui_db[0] = ufsi->i_u1.i_data[0];
 	} else if (inode->i_blocks) {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR); i++)
-			ufs_inode->ui_u2.ui_addr.ui_db[i] = ufsi->i_u1.i_data[i];
+		memcpy(&ufs_inode->ui_u2.ui_addr, ufsi->i_u1.i_data,
+		       sizeof(ufs_inode->ui_u2.ui_addr));
 	}
 	else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
-			ufs_inode->ui_u2.ui_symlink[i] = ufsi->i_u1.i_symlink[i];
+		memcpy(&ufs_inode->ui_u2.ui_symlink, ufsi->i_u1.i_symlink,
+		       sizeof(ufs_inode->ui_u2.ui_symlink));
 	}
 
 	if (!inode->i_nlink)
@@ -825,7 +821,6 @@ static void ufs2_update_inode(struct inode *inode, struct ufs2_inode *ufs_inode)
 {
 	struct super_block *sb = inode->i_sb;
  	struct ufs_inode_info *ufsi = UFS_I(inode);
- 	unsigned i;
 
 	UFSD("ENTER\n");
 	ufs_inode->ui_mode = cpu_to_fs16(sb, inode->i_mode);
@@ -850,11 +845,11 @@ static void ufs2_update_inode(struct inode *inode, struct ufs2_inode *ufs_inode)
 		/* ufs_inode->ui_u2.ui_addr.ui_db[0] = cpu_to_fs32(sb, inode->i_rdev); */
 		ufs_inode->ui_u2.ui_addr.ui_db[0] = ufsi->i_u1.u2_i_data[0];
 	} else if (inode->i_blocks) {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR); i++)
-			ufs_inode->ui_u2.ui_addr.ui_db[i] = ufsi->i_u1.u2_i_data[i];
+		memcpy(&ufs_inode->ui_u2.ui_addr, ufsi->i_u1.u2_i_data,
+		       sizeof(ufs_inode->ui_u2.ui_addr));
 	} else {
-		for (i = 0; i < (UFS_NDADDR + UFS_NINDIR) * 4; i++)
-			ufs_inode->ui_u2.ui_symlink[i] = ufsi->i_u1.i_symlink[i];
+		memcpy(&ufs_inode->ui_u2.ui_symlink, ufsi->i_u1.i_symlink,
+		       sizeof(ufs_inode->ui_u2.ui_symlink));
  	}
 
 	if (!inode->i_nlink)
diff --git a/fs/ufs/ufs.h b/fs/ufs/ufs.h
index 11c0351..69b3427 100644
--- a/fs/ufs/ufs.h
+++ b/fs/ufs/ufs.h
@@ -23,7 +23,7 @@ struct ufs_sb_info {
 struct ufs_inode_info {
 	union {
 		__fs32	i_data[15];
-		__u8	i_symlink[4*15];
+		__u8	i_symlink[2 * 4 * 15];
 		__fs64	u2_i_data[15];
 	} i_u1;
 	__u32	i_flags;

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH 4/4] ufs: copy symlink data into the correct union member

[Duane Griffin]

On Thu, Jan 01, 2009 at 02:36:20AM +0000, Al Viro wrote:
> On Wed, Dec 31, 2008 at 04:50:08PM +0000, Duane Griffin wrote:
> > Copy symlink data into the union member it is accessed through. Although
> > this shouldn't make a difference to behaviour it makes the code easier
> > to follow and grep through. It may also prevent problems if the
> > struct/union definitions change in the future.
> > 
> > Signed-off-by: Duane Griffin <duaneg@dghda.com>
> > ---
> >  fs/ufs/namei.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c
> > index e3a9b1f..3884730 100644
> > --- a/fs/ufs/namei.c
> > +++ b/fs/ufs/namei.c
> > @@ -147,7 +147,7 @@ static int ufs_symlink (struct inode * dir, struct dentry * dentry,
> >  	} else {
> >  		/* fast symlink */
> >  		inode->i_op = &ufs_fast_symlink_inode_operations;
> > -		memcpy((char*)&UFS_I(inode)->i_u1.i_data,symname,l);
> > +		memcpy((char *) &UFS_I(inode)->i_u1.i_symlink, symname, l);
> 
> Just what are these cast and & doing there?  i_symlink is an array of u8,
> for fsck sake; taking its address is a pointless obfuscation.  And memcpy()
> is just as fine with u8 * as it is with char *...

Ahem. Good points. I neglected to clean-up properly, sorry. Will redo
and resend once we've got agreement on the other issues.

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH 1/4] ufs: validate maximum fast symlink size from superblock

[Evgeniy Dushistov]

On Wed, Dec 31, 2008 at 04:50:05PM +0000, Duane Griffin wrote:
> The maximum fast symlink size is set in the superblock of certain types
> of UFS filesystem. Before using it we need to check that it isn't longer
> than the available space we have in the inode.
> 
> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> ---
>  fs/ufs/super.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ufs/super.c b/fs/ufs/super.c
> +		printk(KERN_WARNING "ufs_read_super: excessive maximum fast "
> +		       "symlink size (%u)\n", uspi->s_maxsymlinklen);
> +		uspi->s_maxsymlinklen = maxsymlen;
> +	}
> +

Better and logically to use ufs_warning here, because of it show device id,
in case person has serveral ufs formated partitions and mount happened
during system boot time.

-- 
/Evgeniy