[PATCH v2 1/2] btrfs: Fix superblock csum type check.

[PATCH v2 1/2] btrfs: Fix superblock csum type check.

[Qu Wenruo]

Old csum type check is wrong and can't catch csum_type 1(not supported).

Fix it to avoid hostile 0 division.

Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
Changelog:
v2:
   Fix existing codes other than adding new one.
---
 fs/btrfs/ctree.h   | 1 +
 fs/btrfs/disk-io.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f9c89ca..d6f3aa0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -173,6 +173,7 @@ struct btrfs_ordered_sum;
 
 /* csum types */
 #define BTRFS_CSUM_TYPE_CRC32	0
+#define BTRFS_CSUM_LAST_TYPE	0
 
 static int btrfs_csum_sizes[] = { 4, 0 };
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 639f266..e33a01b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -426,9 +426,9 @@ static int btrfs_check_super_csum(char *raw_disk_sb)
 		}
 	}
 
-	if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
+	if (csum_type > BTRFS_CSUM_LAST_TYPE) {
 		printk(KERN_ERR "BTRFS: unsupported checksum algorithm %u\n",
-				csum_type);
+		       csum_type);
 		ret = 1;
 	}
 
-- 
2.3.5

[PATCH v2 2/2] btrfs: Add extra check for sub_stripes to avoid hostile 0 division attack.

[Qu Wenruo]

Although only RAID10 use sub_stripes, a hostile attack can modify chunk
tree and just add RAID10 bit to a single chunk.
Then btrfs_map_block will trigger a 0 division in kernel and destroy
everything.

Just add extra check when reading chunk from disk.

Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
v2:
   Return -EIO, and add kernel message output.
---
 fs/btrfs/volumes.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 8222f6f..fdcecf7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6061,6 +6061,17 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 	map->stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
 	map->type = btrfs_chunk_type(leaf, chunk);
 	map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
+
+	/* Add extra check to avoid hostile 0 division attack */
+	if (map->type & BTRFS_BLOCK_GROUP_RAID10 &&
+	    map->sub_stripes == 0) {
+		btrfs_warn(root->fs_info,
+			   "RAID10 chunk found but with no sub stripe for bytenr: %llu\n",
+			   logical);
+		free_extent_map(em);
+		return -EIO;
+	}
+
 	for (i = 0; i < num_stripes; i++) {
 		map->stripes[i].physical =
 			btrfs_stripe_offset_nr(leaf, chunk, i);
-- 
2.3.5

Re: [PATCH v2 2/2] btrfs: Add extra check for sub_stripes to avoid hostile 0 division attack.

[David Sterba]

On Fri, Apr 24, 2015 at 09:12:41AM +0800, Qu Wenruo wrote:
> Although only RAID10 use sub_stripes, a hostile attack can modify chunk
> tree and just add RAID10 bit to a single chunk.
> Then btrfs_map_block will trigger a 0 division in kernel and destroy
> everything.
> 
> Just add extra check when reading chunk from disk.
> 
> Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.

[David Sterba]

On Fri, Apr 24, 2015 at 09:12:40AM +0800, Qu Wenruo wrote:
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -173,6 +173,7 @@ struct btrfs_ordered_sum;
>  
>  /* csum types */
>  #define BTRFS_CSUM_TYPE_CRC32	0
> +#define BTRFS_CSUM_LAST_TYPE	0
>  
>  static int btrfs_csum_sizes[] = { 4, 0 };

I'd prefer to fix it by removing the 0 from btrfs_csum_sizes instead of
introducing a define.

Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.

[Qu Wenruo]

-------- Original Message  --------
Subject: Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年04月24日 23:05

> On Fri, Apr 24, 2015 at 09:12:40AM +0800, Qu Wenruo wrote:
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -173,6 +173,7 @@ struct btrfs_ordered_sum;
>>
>>   /* csum types */
>>   #define BTRFS_CSUM_TYPE_CRC32	0
>> +#define BTRFS_CSUM_LAST_TYPE	0
>>
>>   static int btrfs_csum_sizes[] = { 4, 0 };
>
> I'd prefer to fix it by removing the 0 from btrfs_csum_sizes instead of
> introducing a define.
>

Removing the zero seems not help for this case, as some one can still
craft a strange csum_type to access outside the array.

So I introduce the new macro and use the new macro to compare with 
csum_type without acess the array.

Thanks,
Qu

Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.

[David Sterba]

On Mon, Apr 27, 2015 at 08:25:58AM +0800, Qu Wenruo wrote:
> 
> 
> -------- Original Message  --------
> Subject: Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.
> From: David Sterba <dsterba@suse.cz>
> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date: 2015年04月24日 23:05
> 
> > On Fri, Apr 24, 2015 at 09:12:40AM +0800, Qu Wenruo wrote:
> >> --- a/fs/btrfs/ctree.h
> >> +++ b/fs/btrfs/ctree.h
> >> @@ -173,6 +173,7 @@ struct btrfs_ordered_sum;
> >>
> >>   /* csum types */
> >>   #define BTRFS_CSUM_TYPE_CRC32	0
> >> +#define BTRFS_CSUM_LAST_TYPE	0
> >>
> >>   static int btrfs_csum_sizes[] = { 4, 0 };
> >
> > I'd prefer to fix it by removing the 0 from btrfs_csum_sizes instead of
> > introducing a define.
> >
> 
> Removing the zero seems not help for this case, as some one can still
> craft a strange csum_type to access outside the array.

The ARRAY_SIZE will be 1, so if a crafted csum will be anything than 0,
then the check will catch it, no?

> So I introduce the new macro and use the new macro to compare with 
> csum_type without acess the array.

The macro serves the same purpose as the ARRAY_SIZE macro and is always
in sync with the btrfs_csum_size.

Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.

[Qu Wenruo]

-------- Original Message  --------
Subject: Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年04月27日 18:59

> On Mon, Apr 27, 2015 at 08:25:58AM +0800, Qu Wenruo wrote:
>>
>>
>> -------- Original Message  --------
>> Subject: Re: [PATCH v2 1/2] btrfs: Fix superblock csum type check.
>> From: David Sterba <dsterba@suse.cz>
>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> Date: 2015年04月24日 23:05
>>
>>> On Fri, Apr 24, 2015 at 09:12:40AM +0800, Qu Wenruo wrote:
>>>> --- a/fs/btrfs/ctree.h
>>>> +++ b/fs/btrfs/ctree.h
>>>> @@ -173,6 +173,7 @@ struct btrfs_ordered_sum;
>>>>
>>>>    /* csum types */
>>>>    #define BTRFS_CSUM_TYPE_CRC32	0
>>>> +#define BTRFS_CSUM_LAST_TYPE	0
>>>>
>>>>    static int btrfs_csum_sizes[] = { 4, 0 };
>>>
>>> I'd prefer to fix it by removing the 0 from btrfs_csum_sizes instead of
>>> introducing a define.
>>>
>>
>> Removing the zero seems not help for this case, as some one can still
>> craft a strange csum_type to access outside the array.
>
> The ARRAY_SIZE will be 1, so if a crafted csum will be anything than 0,
> then the check will catch it, no?
Oh, I forgot there is ARRAY_SIZE check.
Now deleting the 0 in array is definitely the cleanest fix.

Thanks,
Qu
>
>> So I introduce the new macro and use the new macro to compare with
>> csum_type without acess the array.
>
> The macro serves the same purpose as the ARRAY_SIZE macro and is always
> in sync with the btrfs_csum_size.
> --
> 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
>