[PATCH] btrfs: make thawn time super block check to also verify checksum

[PATCH] btrfs: make thawn time super block check to also verify checksum

[Qu Wenruo]

Previous commit a05d3c915314 ("btrfs: check superblock to ensure the fs
was not modified at thaw time") only checks the content of the super
block, but it doesn't really check if the on-disk super block has a
matching checksum.

This patch will add the checksum verification to thawn time superblock
verification.

This involves the following extra changes:

- Export btrfs_check_super_csum()
  As we need to call it in super.c.

- Change the argument list of btrfs_check_super_csum()
  Instead of passing a char *, directly pass struct btrfs_super_block *
  pointer.

- Verify that our csum type didn't change before checking the csum

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 10 ++++------
 fs/btrfs/disk-io.h |  2 ++
 fs/btrfs/super.c   | 16 ++++++++++++++++
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9f526841c68b..5bf373f606e0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -166,11 +166,9 @@ static bool btrfs_supported_super_csum(u16 csum_type)
  * Return 0 if the superblock checksum type matches the checksum value of that
  * algorithm. Pass the raw disk superblock data.
  */
-static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
-				  char *raw_disk_sb)
+int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
+			   struct btrfs_super_block *disk_sb)
 {
-	struct btrfs_super_block *disk_sb =
-		(struct btrfs_super_block *)raw_disk_sb;
 	char result[BTRFS_CSUM_SIZE];
 	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
 
@@ -181,7 +179,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 	 * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space is
 	 * filled with zeros and is included in the checksum.
 	 */
-	crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
+	crypto_shash_digest(shash, (u8 *)disk_sb + BTRFS_CSUM_SIZE,
 			    BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
 
 	if (memcmp(disk_sb->csum, result, fs_info->csum_size))
@@ -3483,7 +3481,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	 * We want to check superblock checksum, the type is stored inside.
 	 * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k).
 	 */
-	if (btrfs_check_super_csum(fs_info, (u8 *)disk_super)) {
+	if (btrfs_check_super_csum(fs_info, disk_super)) {
 		btrfs_err(fs_info, "superblock checksum mismatch");
 		err = -EINVAL;
 		btrfs_release_disk_super(disk_super);
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 0a77948bb703..70f420044fd5 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -42,6 +42,8 @@ struct extent_buffer *btrfs_find_create_tree_block(
 void btrfs_clean_tree_block(struct extent_buffer *buf);
 void btrfs_clear_oneshot_options(struct btrfs_fs_info *fs_info);
 int btrfs_start_pre_rw_mount(struct btrfs_fs_info *fs_info);
+int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
+			   struct btrfs_super_block *disk_sb);
 int __cold open_ctree(struct super_block *sb,
 	       struct btrfs_fs_devices *fs_devices,
 	       char *options);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 688f7704d3fd..390242aac407 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2555,6 +2555,7 @@ static int check_dev_super(struct btrfs_device *dev)
 {
 	struct btrfs_fs_info *fs_info = dev->fs_info;
 	struct btrfs_super_block *sb;
+	u16 csum_type;
 	int ret = 0;
 
 	/* This should be called with fs still frozen. */
@@ -2569,6 +2570,21 @@ static int check_dev_super(struct btrfs_device *dev)
 	if (IS_ERR(sb))
 		return PTR_ERR(sb);
 
+	/* Verify the csum. */
+	csum_type = btrfs_super_csum_type(sb);
+	if (csum_type != btrfs_super_csum_type(fs_info->super_copy)) {
+		btrfs_err(fs_info, "csum type changed, has %u expect %u",
+			  csum_type, btrfs_super_csum_type(fs_info->super_copy));
+		ret = -EUCLEAN;
+		goto out;
+	}
+
+	if (btrfs_check_super_csum(fs_info, sb)) {
+		btrfs_err(fs_info, "csum for on-disk super block no longer matches");
+		ret = -EUCLEAN;
+		goto out;
+	}
+
 	/* Btrfs_validate_super() includes fsid check against super->fsid. */
 	ret = btrfs_validate_super(fs_info, sb, 0);
 	if (ret < 0)
-- 
2.38.0

Re: [PATCH] btrfs: make thawn time super block check to also verify checksum

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] btrfs: make thawn time super block check to also verify checksum

[David Sterba]

On Tue, Oct 18, 2022 at 09:56:38AM +0800, Qu Wenruo wrote:
> Previous commit a05d3c915314 ("btrfs: check superblock to ensure the fs
> was not modified at thaw time") only checks the content of the super
> block, but it doesn't really check if the on-disk super block has a
> matching checksum.
> 
> This patch will add the checksum verification to thawn time superblock
> verification.
> 
> This involves the following extra changes:
> 
> - Export btrfs_check_super_csum()
>   As we need to call it in super.c.
> 
> - Change the argument list of btrfs_check_super_csum()
>   Instead of passing a char *, directly pass struct btrfs_super_block *
>   pointer.
> 
> - Verify that our csum type didn't change before checking the csum
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to misc-next, thanks. Can you please also send a test case?

> ---
>  fs/btrfs/disk-io.c | 10 ++++------
>  fs/btrfs/disk-io.h |  2 ++
>  fs/btrfs/super.c   | 16 ++++++++++++++++
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 9f526841c68b..5bf373f606e0 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -166,11 +166,9 @@ static bool btrfs_supported_super_csum(u16 csum_type)
>   * Return 0 if the superblock checksum type matches the checksum value of that
>   * algorithm. Pass the raw disk superblock data.
>   */
> -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> -				  char *raw_disk_sb)
> +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> +			   struct btrfs_super_block *disk_sb)

			   const

>  {
> -	struct btrfs_super_block *disk_sb =
> -		(struct btrfs_super_block *)raw_disk_sb;
>  	char result[BTRFS_CSUM_SIZE];
>  	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>  
> @@ -181,7 +179,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>  	 * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space is
>  	 * filled with zeros and is included in the checksum.
>  	 */
> -	crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
> +	crypto_shash_digest(shash, (u8 *)disk_sb + BTRFS_CSUM_SIZE,

				   (const u8 *)

>  			    BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
>  
>  	if (memcmp(disk_sb->csum, result, fs_info->csum_size))

Re: [PATCH] btrfs: make thawn time super block check to also verify checksum

[Qu Wenruo]

On 2022/10/18 21:28, David Sterba wrote:
> On Tue, Oct 18, 2022 at 09:56:38AM +0800, Qu Wenruo wrote:
>> Previous commit a05d3c915314 ("btrfs: check superblock to ensure the fs
>> was not modified at thaw time") only checks the content of the super
>> block, but it doesn't really check if the on-disk super block has a
>> matching checksum.
>>
>> This patch will add the checksum verification to thawn time superblock
>> verification.
>>
>> This involves the following extra changes:
>>
>> - Export btrfs_check_super_csum()
>>    As we need to call it in super.c.
>>
>> - Change the argument list of btrfs_check_super_csum()
>>    Instead of passing a char *, directly pass struct btrfs_super_block *
>>    pointer.
>>
>> - Verify that our csum type didn't change before checking the csum
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Added to misc-next, thanks. Can you please also send a test case?

The test case is a little complex.

Freeze an fs is easy, but there aren't so many easy ways to modify the
super block like modifying it when it's still mounted.

We can do something like wiping the superblock, but it's much harder to
cover all the error paths.

Thanks,
Qu
>
>> ---
>>   fs/btrfs/disk-io.c | 10 ++++------
>>   fs/btrfs/disk-io.h |  2 ++
>>   fs/btrfs/super.c   | 16 ++++++++++++++++
>>   3 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 9f526841c68b..5bf373f606e0 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -166,11 +166,9 @@ static bool btrfs_supported_super_csum(u16 csum_type)
>>    * Return 0 if the superblock checksum type matches the checksum value of that
>>    * algorithm. Pass the raw disk superblock data.
>>    */
>> -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> -				  char *raw_disk_sb)
>> +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> +			   struct btrfs_super_block *disk_sb)
>
> 			   const
>
>>   {
>> -	struct btrfs_super_block *disk_sb =
>> -		(struct btrfs_super_block *)raw_disk_sb;
>>   	char result[BTRFS_CSUM_SIZE];
>>   	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>>
>> @@ -181,7 +179,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>>   	 * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space is
>>   	 * filled with zeros and is included in the checksum.
>>   	 */
>> -	crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
>> +	crypto_shash_digest(shash, (u8 *)disk_sb + BTRFS_CSUM_SIZE,
>
> 				   (const u8 *)
>
>>   			    BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
>>
>>   	if (memcmp(disk_sb->csum, result, fs_info->csum_size))