[PATCH] btrfs: fix a overflowing boundary writing in csum_tree_block

[PATCH] btrfs: fix a overflowing boundary writing in csum_tree_block

[rongqing.li]

From: Li RongQing <roy.qing.li@gmail.com>

It is impossible that csum_size is larger than sizeof(long), but the codes
still add the handler for this condition, like allocate new memory, for
extension. If it becomes true someday, copying csum_size size memory to local
32bit variable found and val will overflow these two variables.

Fix it by returning the max 4 byte checksum.

Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 fs/btrfs/disk-io.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ce8b8b6..cfa2953 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -296,6 +296,8 @@ static int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 		if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
 			u32 val;
 			u32 found = 0;
+
+			csum_size = min_t(u16, csum_size, sizeof(u32));
 			memcpy(&found, result, csum_size);
 
 			read_extent_buffer(buf, &val, 0, csum_size);
-- 
1.7.10.4

Re: [PATCH] btrfs: fix a overflowing boundary writing in csum_tree_block

[Chris Mason]

On 09/09/2014 05:19 AM, rongqing.li@windriver.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> It is impossible that csum_size is larger than sizeof(long), but the codes
> still add the handler for this condition, like allocate new memory, for
> extension. If it becomes true someday, copying csum_size size memory to local
> 32bit variable found and val will overflow these two variables.
> 
> Fix it by returning the max 4 byte checksum.

Thanks for the patch.  I'd rather not silently truncate the copy down
though.  How about a one time check at mount to make sure the value in
the super is reasonable?

-chris

Re: [PATCH] btrfs: fix a overflowing boundary writing in csum_tree_block

[Li RongQing]

Do you means we can handle it like below?  I think it is not better,
if that, csum size can not the expand, and the code in csum_tree_block
seems redundancy;

If you do not want to truncate in first patch, I think we can try to avoid it

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 22a3676..baed6c0 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -879,6 +879,7 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
        u64 subvol_objectid = 0;
        u64 subvol_rootid = 0;
        int error = 0;
+       u16 csum_size;

        if (!(flags & MS_RDONLY))
                mode |= FMODE_WRITE;
@@ -974,6 +975,13 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
                return root;
        }

+       csum_size = btrfs_super_csum_size(root->fs_info->super_copy);
+       if (csum_size > 4) {
+               printk(KERN_ERR "btrfs: csume size is larger than 4\n");
+               deactivate_locked_super(s);
+               return -EINVAL;
+       }
+
        return root;

 error_close_devices:

On Thu, Sep 11, 2014 at 10:02 PM, Chris Mason <clm@fb.com> wrote:
> On 09/09/2014 05:19 AM, rongqing.li@windriver.com wrote:
>> From: Li RongQing <roy.qing.li@gmail.com>
>>
>> It is impossible that csum_size is larger than sizeof(long), but the codes
>> still add the handler for this condition, like allocate new memory, for
>> extension. If it becomes true someday, copying csum_size size memory to local
>> 32bit variable found and val will overflow these two variables.
>>
>> Fix it by returning the max 4 byte checksum.
>
> Thanks for the patch.  I'd rather not silently truncate the copy down
> though.  How about a one time check at mount to make sure the value in
> the super is reasonable?
>
> -chris
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: fix a overflowing boundary writing in csum_tree_block

[David Sterba]

On Thu, Sep 11, 2014 at 10:02:16AM -0400, Chris Mason wrote:
> On 09/09/2014 05:19 AM, rongqing.li@windriver.com wrote:
> > From: Li RongQing <roy.qing.li@gmail.com>
> > 
> > It is impossible that csum_size is larger than sizeof(long), but the codes
> > still add the handler for this condition, like allocate new memory, for
> > extension. If it becomes true someday, copying csum_size size memory to local
> > 32bit variable found and val will overflow these two variables.
> > 
> > Fix it by returning the max 4 byte checksum.
> 
> Thanks for the patch.  I'd rather not silently truncate the copy down
> though.  How about a one time check at mount to make sure the value in
> the super is reasonable?

The check is already there, see btrfs_check_super_csum(), it validates
the csum_type that's later used to find the checksum size.