btrfs: fix a overflowing boundary writing in csum_tree_block

Commit Message

li rongqing Sept. 9, 2014, 9:19 a.m. UTC

From: Li RongQing <[e-mail]>

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 <[e-mail]>
---
 fs/btrfs/disk-io.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Chris Mason Sept. 11, 2014, 2:02 p.m. UTC | #1

On 09/09/2014 05:19 AM, [e-mail] wrote:
> From: Li RongQing <[e-mail]>
> 
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba Sept. 29, 2014, 3:55 p.m. UTC | #2

On Thu, Sep 11, 2014 at 10:02:16AM -0400, Chris Mason wrote:
> On 09/09/2014 05:19 AM, [e-mail] wrote:
> > From: Li RongQing <[e-mail]>
> > 
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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