2009-01-08 22:44:12

by Duane Griffin

[permalink] [raw]
Subject: [PATCH, v2 1/4] ufs: validate maximum fast symlink size from superblock

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 <[email protected]>
---

Version 1 -> 2:
* Replaced printk with ufs_warning

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..9613b8c 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) {
+ ufs_warning(sb, __func__, "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


2009-01-08 22:44:30

by Duane Griffin

[permalink] [raw]
Subject: [PATCH, v2 2/4] ufs: don't truncate longer ufs2 fast symlinks

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.

Note that we copy the entire area instead of using the maxsymlinklen field
from the superblock. This way we will be more robust against corruption (of
the superblock).

While we are at it, use memcpy instead of open-coding it with for loops.

Signed-off-by: Duane Griffin <[email protected]>
---

Let me know if you would prefer me to split the for loop -> memcpy into a
separate patch.

Version 1 -> 2:
* Replaced for loops with memcpy

fs/ufs/inode.c | 37 ++++++++++++++++---------------------
fs/ufs/ufs.h | 2 +-
2 files changed, 17 insertions(+), 22 deletions(-)

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;
--
1.6.0.6

2009-01-08 22:44:49

by Duane Griffin

[permalink] [raw]
Subject: [PATCH, v2 3/4] ufs: ensure fast symlinks are NUL-terminated

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

Signed-off-by: Duane Griffin <[email protected]>
---

checkpatch complains about the lines being over 80 characters, but IMO it is
more readable this way. Let me know if you'd prefer it split it over two
lines, though.

Version 1 -> 2:
* No change

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 ac8b324..3d2512c 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -658,7 +658,8 @@ static int ufs1_read_inode(struct inode *inode, struct ufs_inode *ufs_inode)
sizeof(ufs_inode->ui_u2.ui_addr));
} else {
memcpy(ufsi->i_u1.i_symlink, ufs_inode->ui_u2.ui_symlink,
- sizeof(ufs_inode->ui_u2.ui_symlink));
+ sizeof(ufs_inode->ui_u2.ui_symlink) - 1);
+ ufsi->i_u1.i_symlink[sizeof(ufs_inode->ui_u2.ui_symlink) - 1] = 0;
}
return 0;
}
@@ -706,7 +707,8 @@ static int ufs2_read_inode(struct inode *inode, struct ufs2_inode *ufs2_inode)
sizeof(ufs2_inode->ui_u2.ui_addr));
} else {
memcpy(ufsi->i_u1.i_symlink, ufs2_inode->ui_u2.ui_symlink,
- sizeof(ufs2_inode->ui_u2.ui_symlink));
+ sizeof(ufs2_inode->ui_u2.ui_symlink) - 1);
+ ufsi->i_u1.i_symlink[sizeof(ufs2_inode->ui_u2.ui_symlink) - 1] = 0;
}
return 0;
}
--
1.6.0.6

2009-01-08 22:45:09

by Duane Griffin

[permalink] [raw]
Subject: [PATCH, v2 4/4] ufs: copy symlink data into the correct union member

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 <[email protected]>
---

Version 1 -> 2:
* Remove now obsolete cast; don't take address of the array

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..23119fe 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(UFS_I(inode)->i_u1.i_symlink, symname, l);
inode->i_size = l-1;
}
mark_inode_dirty(inode);
--
1.6.0.6