Dear Ted,
this follows your suggestion from 2009-02-05. The patch is done
analogous to Aneesh Kumar's patches for extent validation. It is based
on 2.6.29-rc7 with Aneesh's patches:
ext4: Add checks to validate extent entries
ext4: Validate extent details only when read from the disk
Kind regards,
Signed-off-by: Thiemo Nagel <[email protected]>
And this one should limit the checks to freshly-read block only.
Signed-off-by: Thiemo Nagel <[email protected]>
Oops. Fixed comment.
Signed-off-by: Thiemo Nagel <[email protected]>
--- linux-2.6.29-rc7/fs/ext4/inode.c.orig 2009-03-20 11:35:45.000000000 +0100
+++ linux-2.6.29-rc7/fs/ext4/inode.c 2009-03-20 13:48:25.000000000 +0100
@@ -371,6 +371,34 @@
return n;
}
+static int __ext4_check_blockref(const char *function, struct inode *inode,
+ unsigned int *p, unsigned int max) {
+
+ unsigned int maxblocks = ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es);
+ unsigned int *bref = p;
+ while (bref < p+max) {
+ if (unlikely(*bref >= maxblocks)) {
+ ext4_error(inode->i_sb, function,
+ "block reference %u >= max (%u) "
+ "in inode #%lu, offset=%u",
+ *bref, maxblocks,
+ inode->i_ino, bref-p);
+ return -EIO;
+ }
+ bref++;
+ }
+ return 0;
+}
+
+
+#define ext4_check_indirect_blockref(inode, bh) \
+ __ext4_check_blockref(__func__, inode, (__le32 *)(bh)->b_data, \
+ EXT4_ADDR_PER_BLOCK((inode)->i_sb))
+
+#define ext4_check_inode_blockref(inode) \
+ __ext4_check_blockref(__func__, inode, EXT4_I(inode)->i_data, \
+ EXT4_NDIR_BLOCKS)
+
/**
* ext4_get_branch - read the chain of indirect blocks leading to data
* @inode: inode in question
@@ -418,6 +446,9 @@
bh = sb_bread(sb, le32_to_cpu(p->key));
if (!bh)
goto failure;
+ if (ext4_check_indirect_blockref(inode, bh))
+ goto failure;
+
add_chain(++p, bh, (__le32 *)bh->b_data + *++offsets);
/* Reader: end */
if (!p->key)
@@ -4302,11 +4333,13 @@
if (ei->i_flags & EXT4_EXTENTS_FL) {
/* Validate extent which is part of inode */
ret = ext4_ext_check_inode(inode);
- if (ret) {
- brelse(bh);
- goto bad_inode;
- }
Thanks, I've combined these your two patches ("add checks of block
references for non-extent inodes" and "check block references only
when read from disk") into a single patch and added it to the ext4
patch queue.
- Ted
On Fri, Mar 20, 2009 at 03:15:56PM +0100, Thiemo Nagel wrote:
> Dear Ted,
>
> I'm sending an improved patch as I've come to the conclusion that the
> previous patch is too lenient in two ways:
> * off-by-one in the check of the upper block limit
> * it shouldn't stop when encountering a reference to block number zero
> because, if I'm not mistaken, references behind it still might be
> accessed in sparse files / when seeking behind the end of a file.
Note: this patch has a rather fatal flaw; it doens't check to make
sure that file is a regular file or directory first. If you have a
fast symlink, ext4_iget() will blow up since it's a non-extent file,
but the i_blocks[] will contain an ASCII string.
I'll fix up your patch to deal add the appropriate checking.
- Ted
On Fri, Mar 20, 2009 at 03:15:56PM +0100, Thiemo Nagel wrote:
> Dear Ted,
>
> I'm sending an improved patch as I've come to the conclusion that the
> previous patch is too lenient in two ways:
> * off-by-one in the check of the upper block limit
> * it shouldn't stop when encountering a reference to block number zero
> because, if I'm not mistaken, references behind it still might be
> accessed in sparse files / when seeking behind the end of a file.
>
> On the other hand, I decided to drop the check against
> s_first_data_block at the low end to improve performance, since the
> purpose of the patch is to prevent access to blocks outside the
> filesystem, and not to do the best-possible consistency check against
> indirect blocks, which probably is better done in fsck.
>
> Anyways, in case you would be interested in having more checks here (eg.
> as a compile-time option), I have available a more sophisticated patch
> which also checks for non-zero block references behind the end of the
> file.
>
> Kind regards,
>
> Signed-off-by: Thiemo Nagel <[email protected]>
>
>
> --- linux-2.6.29-rc7/fs/ext4/inode.c.orig 2009-03-20 11:35:45.000000000 +0100
> +++ linux-2.6.29-rc7/fs/ext4/inode.c 2009-03-20 13:48:25.000000000 +0100
> @@ -371,6 +371,34 @@
> return n;
> }
>
> +static int __ext4_check_blockref(const char *function, struct inode *inode,
> + unsigned int *p, unsigned int max) {
> +
> + unsigned int maxblocks = ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es);
> + unsigned int *bref = p;
> + while (bref < p+max) {
> + if (unlikely(*bref >= maxblocks)) {
> + ext4_error(inode->i_sb, function,
> + "block reference %u >= max (%u) "
> + "in inode #%lu, offset=%u",
> + *bref, maxblocks,
> + inode->i_ino, bref-p);
> + return -EIO;
> + }
> + bref++;
> + }
> + return 0;
> +}
> +
> +
> +#define ext4_check_indirect_blockref(inode, bh) \
> + __ext4_check_blockref(__func__, inode, (__le32 *)(bh)->b_data, \
> + EXT4_ADDR_PER_BLOCK((inode)->i_sb))
> +
> +#define ext4_check_inode_blockref(inode) \
> + __ext4_check_blockref(__func__, inode, EXT4_I(inode)->i_data, \
> + EXT4_NDIR_BLOCKS)
> +
> /**
> * ext4_get_branch - read the chain of indirect blocks leading to data
> * @inode: inode in question
> @@ -418,6 +446,9 @@
> bh = sb_bread(sb, le32_to_cpu(p->key));
> if (!bh)
> goto failure;
> + if (ext4_check_indirect_blockref(inode, bh))
> + goto failure;
> +
Since on errors=continue we are not adding the bh to the chain. We leak
a buffer_head reference here. I guess we need a put_bh before goto
failure.
-aneesh
Dear Ted, Aneesh, Stephen,
thanks a lot for your comments and fixes! The attached patch (applied
on-top) should fix the
* buffer head leak spotted by Aneesh
* warning reported by Stephen
Kind regards,
Signed-off-by: Thiemo Nagel <[email protected]>
On Tue, Mar 31, 2009 at 10:41:27AM +0200, Thiemo Nagel wrote:
> Dear Ted, Aneesh, Stephen,
>
> thanks a lot for your comments and fixes! The attached patch (applied
> on-top) should fix the
> * buffer head leak spotted by Aneesh
> * warning reported by Stephen
>
> Kind regards,
Thanks, I've merged this into your patch in the ext4 patch queue.
- Ted
Theodore Tso wrote:
> On Tue, Mar 31, 2009 at 10:41:27AM +0200, Thiemo Nagel wrote:
>> Dear Ted, Aneesh, Stephen,
>>
>> thanks a lot for your comments and fixes! The attached patch (applied
>> on-top) should fix the
>> * buffer head leak spotted by Aneesh
>> * warning reported by Stephen
>
> Thanks, I've merged this into your patch in the ext4 patch queue.
Thank you!
BTW: Do you want this patch also for ext2/3 ?
Kind regards,
Thiemo