[PATCH RFC 0/7] fs: Debug config option to disable filesystem checksum verification for fuzzing

[PATCH RFC 0/7] fs: Debug config option to disable filesystem checksum verification for fuzzing

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

Fuzzing is a proven technique to discover exploitable bugs in the Linux
kernel. But fuzzing filesystems is tricky: highly structured disk images
use redundant checksums to verify data integrity. Therefore,
randomly-mutated images are quickly rejected as corrupt, testing only
error-handling code effectively.

The Janus [1] and Hydra [2] projects probe filesystem code deeply by
correcting checksums after mutation. But their ad-hoc
checksum-correcting code supports only a few filesystems, and it is
difficult to support new ones - requiring significant duplication of
filesystem logic which must also be kept in sync with upstream changes.
Corrected checksums cannot be guaranteed to be valid, and reusing this
code across different fuzzing frameworks is non-trivial.

Instead, this RFC suggests a config option:
`DISABLE_FS_CSUM_VERIFICATION`. When it is enabled, all filesystems
should bypass redundant checksum verification, proceeding as if
checksums are valid. Setting of checksums should be unaffected. Mutated
images will no longer be rejected due to invalid checksums, allowing
testing of deeper code paths. Though some filesystems implement their
own flags to disable some checksums, this option should instead disable
all checksums for all filesystems uniformly. Critically, any bugs found
remain reproducible on production systems: redundant checksums in
mutated images can be fixed up to satisfy verification.

The patches below suggest a potential implementation for a few
filesystems, though we may have missed some checksums. The option
requires `DEBUG_KERNEL` and is not intended for production systems.

The first user of the option would be syzbot. We ran preliminary local
syzkaller tests to compare behaviour with and without these patches.
With the patches, we found a 19% increase in coverage, as well as many
new crash types and increases in the total number of crashes:

Filesystem | % new crash types | % increase in crashes
—----------|-------------------|----------------------
  ext4     |        60%        |         1400%
  btrfs    |        25%        |         185%
  f2fs     |        63%        |         16%


[1] Fuzzing file systems via two-dimensional input space exploration,
    Xu et al., 2019, IEEE Symposium on Security and Privacy,
    doi: 10.1109/SP.2019.00035
[2] Finding semantic bugs in file systems with an extensible fuzzing
    framework, Kim et al., 2019, ACM Symposium on Operating Systems
    Principles, doi: 10.1145/3341301.3359662


Hrutvik Kanabar (7):
  fs: create `DISABLE_FS_CSUM_VERIFICATION` config option
  fs/ext4: support `DISABLE_FS_CSUM_VERIFICATION` config option
  fs/btrfs: support `DISABLE_FS_CSUM_VERIFICATION` config option
  fs/exfat: support `DISABLE_FS_CSUM_VERIFICATION` config option
  fs/xfs: support `DISABLE_FS_CSUM_VERIFICATION` config option
  fs/ntfs: support `DISABLE_FS_CSUM_VERIFICATION` config option
  fs/f2fs: support `DISABLE_FS_CSUM_VERIFICATION` config option

 fs/Kconfig.debug            | 20 ++++++++++++++++++++
 fs/btrfs/check-integrity.c  |  3 ++-
 fs/btrfs/disk-io.c          |  6 ++++--
 fs/btrfs/free-space-cache.c |  3 ++-
 fs/btrfs/inode.c            |  3 ++-
 fs/btrfs/scrub.c            |  9 ++++++---
 fs/exfat/nls.c              |  3 ++-
 fs/exfat/super.c            |  3 +++
 fs/ext4/bitmap.c            |  6 ++++--
 fs/ext4/extents.c           |  3 ++-
 fs/ext4/inode.c             |  3 ++-
 fs/ext4/ioctl.c             |  3 ++-
 fs/ext4/mmp.c               |  3 ++-
 fs/ext4/namei.c             |  6 ++++--
 fs/ext4/orphan.c            |  3 ++-
 fs/ext4/super.c             |  6 ++++--
 fs/ext4/xattr.c             |  3 ++-
 fs/f2fs/checkpoint.c        |  3 ++-
 fs/f2fs/compress.c          |  3 ++-
 fs/f2fs/f2fs.h              |  2 ++
 fs/f2fs/inode.c             |  3 +++
 fs/ntfs/super.c             |  3 ++-
 fs/xfs/libxfs/xfs_cksum.h   |  5 ++++-
 lib/Kconfig.debug           |  6 ++++++
 24 files changed, 86 insertions(+), 25 deletions(-)
 create mode 100644 fs/Kconfig.debug

-- 
2.38.0.413.g74048e4d9e-goog

[PATCH RFC 1/7] fs: create `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When implemented and enabled, this should circumvent all redundant
checksum verification in filesystem code. However, setting of checksums
is not affected.

The aim is to aid fuzzing efforts which randomly mutate disk images and
so invalidate checksums.  Checksum verification often rejects these
mutated disk images, hindering fuzzer coverage of deep code paths. By
disabling checksum verification, all mutated images are considered valid
and so exploration of interesting code paths can continue.

This option requires the `DEBUG_KERNEL` option, and is not intended to
be used on production systems.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/Kconfig.debug  | 20 ++++++++++++++++++++
 lib/Kconfig.debug |  6 ++++++
 2 files changed, 26 insertions(+)
 create mode 100644 fs/Kconfig.debug

diff --git a/fs/Kconfig.debug b/fs/Kconfig.debug
new file mode 100644
index 000000000000..bc1018e3d580
--- /dev/null
+++ b/fs/Kconfig.debug
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config DISABLE_FS_CSUM_VERIFICATION
+	bool "Disable redundant checksum verification for filesystems"
+	depends on DEBUG_KERNEL
+	help
+	  Disable filesystem checksum verification for checksums which can be
+	  trivially recomputed from the on-disk data (i.e. no encryption).
+	  Note that this does not affect setting of checksums.
+
+	  This option is useful for filesystem testing. For example, fuzzing
+	  with randomly mutated disk images can uncover bugs exploitable by
+	  specially-crafted disks. Redundant checksums are orthogonal to these
+	  exploits, as they can be recomputed for crafted disks. However, for
+	  testing it is more reliable to disable checksums within the kernel
+	  than to maintain image generators which faithfully reimplement
+	  per-filesystem checksum recomputation.
+
+	  Say N if you are unsure. Disable this for production systems!
+
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 73178b0e43a4..4689ae527993 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -979,6 +979,12 @@ source "lib/Kconfig.kmsan"
 
 endmenu # "Memory Debugging"
 
+menu "Filesystem Debugging"
+
+source "fs/Kconfig.debug"
+
+endmenu # "Filesystem Debugging"
+
 config DEBUG_SHIRQ
 	bool "Debug shared IRQ handlers"
 	depends on DEBUG_KERNEL
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH RFC 2/7] fs/ext4: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checks in key
checksum verification functions.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/ext4/bitmap.c  | 6 ++++--
 fs/ext4/extents.c | 3 ++-
 fs/ext4/inode.c   | 3 ++-
 fs/ext4/ioctl.c   | 3 ++-
 fs/ext4/mmp.c     | 3 ++-
 fs/ext4/namei.c   | 6 ++++--
 fs/ext4/orphan.c  | 3 ++-
 fs/ext4/super.c   | 6 ++++--
 fs/ext4/xattr.c   | 3 ++-
 9 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/bitmap.c b/fs/ext4/bitmap.c
index f63e028c638c..04ce8e4149ee 100644
--- a/fs/ext4/bitmap.c
+++ b/fs/ext4/bitmap.c
@@ -24,7 +24,8 @@ int ext4_inode_bitmap_csum_verify(struct super_block *sb, ext4_group_t group,
 	__u32 provided, calculated;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	if (!ext4_has_metadata_csum(sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(sb))
 		return 1;
 
 	provided = le16_to_cpu(gdp->bg_inode_bitmap_csum_lo);
@@ -63,7 +64,8 @@ int ext4_block_bitmap_csum_verify(struct super_block *sb, ext4_group_t group,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	int sz = EXT4_CLUSTERS_PER_GROUP(sb) / 8;
 
-	if (!ext4_has_metadata_csum(sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(sb))
 		return 1;
 
 	provided = le16_to_cpu(gdp->bg_block_bitmap_csum_lo);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index f1956288307f..c1b7c8f4862c 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -63,7 +63,8 @@ static int ext4_extent_block_csum_verify(struct inode *inode,
 {
 	struct ext4_extent_tail *et;
 
-	if (!ext4_has_metadata_csum(inode->i_sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(inode->i_sb))
 		return 1;
 
 	et = find_ext4_extent_tail(eh);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2b5ef1b64249..8ec8214f1423 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -86,7 +86,8 @@ static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw,
 {
 	__u32 provided, calculated;
 
-	if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
 	    cpu_to_le32(EXT4_OS_LINUX) ||
 	    !ext4_has_metadata_csum(inode->i_sb))
 		return 1;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 4d49c5cfb690..bae33cd83d05 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -142,7 +142,8 @@ static int ext4_update_backup_sb(struct super_block *sb,
 
 	es = (struct ext4_super_block *) (bh->b_data + offset);
 	lock_buffer(bh);
-	if (ext4_has_metadata_csum(sb) &&
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    ext4_has_metadata_csum(sb) &&
 	    es->s_checksum != ext4_superblock_csum(sb, es)) {
 		ext4_msg(sb, KERN_ERR, "Invalid checksum for backup "
 		"superblock %llu\n", sb_block);
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 9af68a7ecdcf..605f1867958d 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -21,7 +21,8 @@ static __le32 ext4_mmp_csum(struct super_block *sb, struct mmp_struct *mmp)
 
 static int ext4_mmp_csum_verify(struct super_block *sb, struct mmp_struct *mmp)
 {
-	if (!ext4_has_metadata_csum(sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(sb))
 		return 1;
 
 	return mmp->mmp_checksum == ext4_mmp_csum(sb, mmp);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index d5daaf41e1fc..84a59052c51d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -396,7 +396,8 @@ int ext4_dirblock_csum_verify(struct inode *inode, struct buffer_head *bh)
 {
 	struct ext4_dir_entry_tail *t;
 
-	if (!ext4_has_metadata_csum(inode->i_sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(inode->i_sb))
 		return 1;
 
 	t = get_dirent_tail(inode, bh);
@@ -491,7 +492,8 @@ static int ext4_dx_csum_verify(struct inode *inode,
 	struct dx_tail *t;
 	int count_offset, limit, count;
 
-	if (!ext4_has_metadata_csum(inode->i_sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(inode->i_sb))
 		return 1;
 
 	c = get_dx_countlimit(inode, dirent, &count_offset);
diff --git a/fs/ext4/orphan.c b/fs/ext4/orphan.c
index 69a9cf9137a6..8a488d5521cb 100644
--- a/fs/ext4/orphan.c
+++ b/fs/ext4/orphan.c
@@ -537,7 +537,8 @@ static int ext4_orphan_file_block_csum_verify(struct super_block *sb,
 	struct ext4_orphan_block_tail *ot;
 	__le64 dsk_block_nr = cpu_to_le64(bh->b_blocknr);
 
-	if (!ext4_has_metadata_csum(sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(sb))
 		return 1;
 
 	ot = ext4_orphan_block_tail(sb, bh);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d733db8a0b02..cb6e53163441 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -287,7 +287,8 @@ __le32 ext4_superblock_csum(struct super_block *sb,
 static int ext4_superblock_csum_verify(struct super_block *sb,
 				       struct ext4_super_block *es)
 {
-	if (!ext4_has_metadata_csum(sb))
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    !ext4_has_metadata_csum(sb))
 		return 1;
 
 	return es->s_checksum == ext4_superblock_csum(sb, es);
@@ -3198,7 +3199,8 @@ static __le16 ext4_group_desc_csum(struct super_block *sb, __u32 block_group,
 int ext4_group_desc_csum_verify(struct super_block *sb, __u32 block_group,
 				struct ext4_group_desc *gdp)
 {
-	if (ext4_has_group_desc_csum(sb) &&
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    ext4_has_group_desc_csum(sb) &&
 	    (gdp->bg_checksum != ext4_group_desc_csum(sb, block_group, gdp)))
 		return 0;
 
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 36d6ba7190b6..b22a0f282474 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -154,7 +154,8 @@ static int ext4_xattr_block_csum_verify(struct inode *inode,
 	struct ext4_xattr_header *hdr = BHDR(bh);
 	int ret = 1;
 
-	if (ext4_has_metadata_csum(inode->i_sb)) {
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    ext4_has_metadata_csum(inode->i_sb)) {
 		lock_buffer(bh);
 		ret = (hdr->h_checksum == ext4_xattr_block_csum(inode,
 							bh->b_blocknr, hdr));
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH RFC 3/7] fs/btrfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
verification.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/btrfs/check-integrity.c  | 3 ++-
 fs/btrfs/disk-io.c          | 6 ++++--
 fs/btrfs/free-space-cache.c | 3 ++-
 fs/btrfs/inode.c            | 3 ++-
 fs/btrfs/scrub.c            | 9 ++++++---
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 98c6e5feab19..eab82593a325 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -1671,7 +1671,8 @@ static noinline_for_stack int btrfsic_test_for_metadata(
 		crypto_shash_update(shash, data, sublen);
 	}
 	crypto_shash_final(shash, csum);
-	if (memcmp(csum, h->csum, fs_info->csum_size))
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(csum, h->csum, fs_info->csum_size))
 		return 1;
 
 	return 0; /* is metadata */
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a2da9313c694..7cd909d44b24 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -184,7 +184,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 	crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
 			    BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
 
-	if (memcmp(disk_sb->csum, result, fs_info->csum_size))
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(disk_sb->csum, result, fs_info->csum_size))
 		return 1;
 
 	return 0;
@@ -494,7 +495,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
 	header_csum = page_address(eb->pages[0]) +
 		get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
 
-	if (memcmp(result, header_csum, csum_size) != 0) {
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(result, header_csum, csum_size) != 0) {
 		btrfs_warn_rl(fs_info,
 "checksum verify failed on logical %llu mirror %u wanted " CSUM_FMT " found " CSUM_FMT " level %d",
 			      eb->start, eb->read_mirror,
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index f4023651dd68..203c8a9076a6 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -574,7 +574,8 @@ static int io_ctl_check_crc(struct btrfs_io_ctl *io_ctl, int index)
 	io_ctl_map_page(io_ctl, 0);
 	crc = btrfs_crc32c(crc, io_ctl->orig + offset, PAGE_SIZE - offset);
 	btrfs_crc32c_final(crc, (u8 *)&crc);
-	if (val != crc) {
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    val != crc) {
 		btrfs_err_rl(io_ctl->fs_info,
 			"csum mismatch on free space cache");
 		io_ctl_unmap_page(io_ctl);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b0807c59e321..1a49d897b5c1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3434,7 +3434,8 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
 	crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
 	kunmap_local(kaddr);
 
-	if (memcmp(csum, csum_expected, fs_info->csum_size))
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(csum, csum_expected, fs_info->csum_size))
 		return -EIO;
 	return 0;
 }
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index f260c53829e5..a7607b492f47 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1997,7 +1997,8 @@ static int scrub_checksum_data(struct scrub_block *sblock)
 
 	crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
 
-	if (memcmp(csum, sector->csum, fs_info->csum_size))
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(csum, sector->csum, fs_info->csum_size))
 		sblock->checksum_error = 1;
 	return sblock->checksum_error;
 }
@@ -2062,7 +2063,8 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
 	}
 
 	crypto_shash_final(shash, calculated_csum);
-	if (memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
 		sblock->checksum_error = 1;
 
 	return sblock->header_error || sblock->checksum_error;
@@ -2099,7 +2101,8 @@ static int scrub_checksum_super(struct scrub_block *sblock)
 	crypto_shash_digest(shash, kaddr + BTRFS_CSUM_SIZE,
 			BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, calculated_csum);
 
-	if (memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
 		++fail_cor;
 
 	return fail_cor + fail_gen;
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH RFC 3/7] fs/btrfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Qu Wenruo]

On 2022/10/14 16:48, Hrutvik Kanabar wrote:
> From: Hrutvik Kanabar <hrutvik@google.com>
> 
> When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
> verification.
> 
> Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>

I always want more fuzz for btrfs, so overall this is pretty good.

But there are some comments related to free space cache part.

Despite the details, I'm wondering would it be possible for your fuzzing 
tool to do a better job at user space? Other than relying on loosen 
checks from kernel?

For example, implement a (mostly) read-only tool to do the following 
workload:

- Open the fs
   Including understand the checksum algo, how to re-generate the csum.

- Read out the used space bitmap
   In btrfs case, it's going to read the extent tree, process the
   backrefs items.

- Choose the victim sectors and corrupt them
   Obviously, vitims should be choosen from above used space bitmap.

- Re-calculate the checksum for above corrupted sectors
   For btrfs, if it's a corrupted metadata, re-calculate the checksum.

By this, we can avoid such change to kernel, and still get a much better 
coverage.

If you need some help on such user space tool, I'm pretty happy to 
provide help.

> ---
>   fs/btrfs/check-integrity.c  | 3 ++-
>   fs/btrfs/disk-io.c          | 6 ++++--
>   fs/btrfs/free-space-cache.c | 3 ++-
>   fs/btrfs/inode.c            | 3 ++-
>   fs/btrfs/scrub.c            | 9 ++++++---
>   5 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index 98c6e5feab19..eab82593a325 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -1671,7 +1671,8 @@ static noinline_for_stack int btrfsic_test_for_metadata(
>   		crypto_shash_update(shash, data, sublen);
>   	}
>   	crypto_shash_final(shash, csum);
> -	if (memcmp(csum, h->csum, fs_info->csum_size))
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(csum, h->csum, fs_info->csum_size))
>   		return 1;
>   
>   	return 0; /* is metadata */
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index a2da9313c694..7cd909d44b24 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -184,7 +184,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>   	crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
>   			    BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
>   
> -	if (memcmp(disk_sb->csum, result, fs_info->csum_size))
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(disk_sb->csum, result, fs_info->csum_size))
>   		return 1;
>   
>   	return 0;
> @@ -494,7 +495,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>   	header_csum = page_address(eb->pages[0]) +
>   		get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
>   
> -	if (memcmp(result, header_csum, csum_size) != 0) {
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(result, header_csum, csum_size) != 0) {

I believe this is the main thing fuzzing would take advantage of.

It would be much better if this is the only override...

>   		btrfs_warn_rl(fs_info,
>   "checksum verify failed on logical %llu mirror %u wanted " CSUM_FMT " found " CSUM_FMT " level %d",
>   			      eb->start, eb->read_mirror,
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index f4023651dd68..203c8a9076a6 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -574,7 +574,8 @@ static int io_ctl_check_crc(struct btrfs_io_ctl *io_ctl, int index)
>   	io_ctl_map_page(io_ctl, 0);
>   	crc = btrfs_crc32c(crc, io_ctl->orig + offset, PAGE_SIZE - offset);
>   	btrfs_crc32c_final(crc, (u8 *)&crc);
> -	if (val != crc) {
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    val != crc) {

I'm already seeing this to cause problems, especially for btrfs.

Btrfs has a very strong dependency on free space tracing, as all of our 
metadata (and data by default) relies on COW to keep the fs consistent.

I tried a lot of different methods in the past to make sure we won't 
write into previously used space, but it's causing a lot of performance 
impact.

Unlike tree-checker, we can not easily got a centerlized space to handle 
all the free space cross-check thing (thus it's only verified by things 
like btrfs-check).

Furthermore, even if you skip this override, with latest default 
free-space-tree feature, free space info is stored in regular btrfs 
metadata (tree blocks), with regular metadata checksum protection.

Thus I'm pretty sure we will have tons of reports on this, and 
unfortunately we can only go whac-a-mole way for it.

>   		btrfs_err_rl(io_ctl->fs_info,
>   			"csum mismatch on free space cache");
>   		io_ctl_unmap_page(io_ctl);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b0807c59e321..1a49d897b5c1 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3434,7 +3434,8 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
>   	crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
>   	kunmap_local(kaddr);
>   
> -	if (memcmp(csum, csum_expected, fs_info->csum_size))
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(csum, csum_expected, fs_info->csum_size))

This skips data csum check, I don't know how valueable it is, but this 
should be harmless mostly.

If we got reports related to this, it would be a nice surprise.

>   		return -EIO;
>   	return 0;
>   }
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index f260c53829e5..a7607b492f47 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -1997,7 +1997,8 @@ static int scrub_checksum_data(struct scrub_block *sblock)
>   
>   	crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
>   
> -	if (memcmp(csum, sector->csum, fs_info->csum_size))
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(csum, sector->csum, fs_info->csum_size))

Same as data csum verification overide.
Not necessary/useful but good to have.

>   		sblock->checksum_error = 1;
>   	return sblock->checksum_error;
>   }
> @@ -2062,7 +2063,8 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
>   	}
>   
>   	crypto_shash_final(shash, calculated_csum);
> -	if (memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))

This is much less valueable, since it's only affecting scrub, and scrub 
itself is already very little checking the content of metadata.

Thanks,
Qu

>   		sblock->checksum_error = 1;
>   
>   	return sblock->header_error || sblock->checksum_error;
> @@ -2099,7 +2101,8 @@ static int scrub_checksum_super(struct scrub_block *sblock)
>   	crypto_shash_digest(shash, kaddr + BTRFS_CSUM_SIZE,
>   			BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, calculated_csum);
>   
> -	if (memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
> +	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> +	    memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
>   		++fail_cor;
>   
>   	return fail_cor + fail_gen;

Re: [PATCH RFC 3/7] fs/btrfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Dmitry Vyukov]

On Fri, 14 Oct 2022 at 12:24, 'Qu Wenruo' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> On 2022/10/14 16:48, Hrutvik Kanabar wrote:
> > From: Hrutvik Kanabar <hrutvik@google.com>
> >
> > When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
> > verification.
> >
> > Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
>
> I always want more fuzz for btrfs, so overall this is pretty good.
>
> But there are some comments related to free space cache part.
>
> Despite the details, I'm wondering would it be possible for your fuzzing
> tool to do a better job at user space? Other than relying on loosen
> checks from kernel?
>
> For example, implement a (mostly) read-only tool to do the following
> workload:
>
> - Open the fs
>    Including understand the checksum algo, how to re-generate the csum.
>
> - Read out the used space bitmap
>    In btrfs case, it's going to read the extent tree, process the
>    backrefs items.
>
> - Choose the victim sectors and corrupt them
>    Obviously, vitims should be choosen from above used space bitmap.
>
> - Re-calculate the checksum for above corrupted sectors
>    For btrfs, if it's a corrupted metadata, re-calculate the checksum.
>
> By this, we can avoid such change to kernel, and still get a much better
> coverage.
>
> If you need some help on such user space tool, I'm pretty happy to
> provide help.
>
> > ---
> >   fs/btrfs/check-integrity.c  | 3 ++-
> >   fs/btrfs/disk-io.c          | 6 ++++--
> >   fs/btrfs/free-space-cache.c | 3 ++-
> >   fs/btrfs/inode.c            | 3 ++-
> >   fs/btrfs/scrub.c            | 9 ++++++---
> >   5 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> > index 98c6e5feab19..eab82593a325 100644
> > --- a/fs/btrfs/check-integrity.c
> > +++ b/fs/btrfs/check-integrity.c
> > @@ -1671,7 +1671,8 @@ static noinline_for_stack int btrfsic_test_for_metadata(
> >               crypto_shash_update(shash, data, sublen);
> >       }
> >       crypto_shash_final(shash, csum);
> > -     if (memcmp(csum, h->csum, fs_info->csum_size))
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(csum, h->csum, fs_info->csum_size))
> >               return 1;
> >
> >       return 0; /* is metadata */
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index a2da9313c694..7cd909d44b24 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -184,7 +184,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> >       crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
> >                           BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
> >
> > -     if (memcmp(disk_sb->csum, result, fs_info->csum_size))
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(disk_sb->csum, result, fs_info->csum_size))
> >               return 1;
> >
> >       return 0;
> > @@ -494,7 +495,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> >       header_csum = page_address(eb->pages[0]) +
> >               get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
> >
> > -     if (memcmp(result, header_csum, csum_size) != 0) {
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(result, header_csum, csum_size) != 0) {
>
> I believe this is the main thing fuzzing would take advantage of.
>
> It would be much better if this is the only override...
>
> >               btrfs_warn_rl(fs_info,
> >   "checksum verify failed on logical %llu mirror %u wanted " CSUM_FMT " found " CSUM_FMT " level %d",
> >                             eb->start, eb->read_mirror,
> > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> > index f4023651dd68..203c8a9076a6 100644
> > --- a/fs/btrfs/free-space-cache.c
> > +++ b/fs/btrfs/free-space-cache.c
> > @@ -574,7 +574,8 @@ static int io_ctl_check_crc(struct btrfs_io_ctl *io_ctl, int index)
> >       io_ctl_map_page(io_ctl, 0);
> >       crc = btrfs_crc32c(crc, io_ctl->orig + offset, PAGE_SIZE - offset);
> >       btrfs_crc32c_final(crc, (u8 *)&crc);
> > -     if (val != crc) {
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         val != crc) {
>
> I'm already seeing this to cause problems, especially for btrfs.
>
> Btrfs has a very strong dependency on free space tracing, as all of our
> metadata (and data by default) relies on COW to keep the fs consistent.
>
> I tried a lot of different methods in the past to make sure we won't
> write into previously used space, but it's causing a lot of performance
> impact.
>
> Unlike tree-checker, we can not easily got a centerlized space to handle
> all the free space cross-check thing (thus it's only verified by things
> like btrfs-check).
>
> Furthermore, even if you skip this override, with latest default
> free-space-tree feature, free space info is stored in regular btrfs
> metadata (tree blocks), with regular metadata checksum protection.
>
> Thus I'm pretty sure we will have tons of reports on this, and
> unfortunately we can only go whac-a-mole way for it.

Hi Qu,

I don't fully understand what you mean. Could you please elaborate?

Do you mean that btrfs uses this checksum check to detect blocks that
were written to w/o updating the checksum?




> >               btrfs_err_rl(io_ctl->fs_info,
> >                       "csum mismatch on free space cache");
> >               io_ctl_unmap_page(io_ctl);
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index b0807c59e321..1a49d897b5c1 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -3434,7 +3434,8 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
> >       crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
> >       kunmap_local(kaddr);
> >
> > -     if (memcmp(csum, csum_expected, fs_info->csum_size))
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(csum, csum_expected, fs_info->csum_size))
>
> This skips data csum check, I don't know how valueable it is, but this
> should be harmless mostly.
>
> If we got reports related to this, it would be a nice surprise.
>
> >               return -EIO;
> >       return 0;
> >   }
> > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> > index f260c53829e5..a7607b492f47 100644
> > --- a/fs/btrfs/scrub.c
> > +++ b/fs/btrfs/scrub.c
> > @@ -1997,7 +1997,8 @@ static int scrub_checksum_data(struct scrub_block *sblock)
> >
> >       crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
> >
> > -     if (memcmp(csum, sector->csum, fs_info->csum_size))
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(csum, sector->csum, fs_info->csum_size))
>
> Same as data csum verification overide.
> Not necessary/useful but good to have.
>
> >               sblock->checksum_error = 1;
> >       return sblock->checksum_error;
> >   }
> > @@ -2062,7 +2063,8 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
> >       }
> >
> >       crypto_shash_final(shash, calculated_csum);
> > -     if (memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
>
> This is much less valueable, since it's only affecting scrub, and scrub
> itself is already very little checking the content of metadata.

Could you please elaborate here as well?
This is less valuable from what perspective?
The data loaded from disk can have any combination of
(correct/incorrect metadata) x (correct/incorrect checksum).
Correctness of metadata and checksum are effectively orthogonal,
right?



> Thanks,
> Qu
>
> >               sblock->checksum_error = 1;
> >
> >       return sblock->header_error || sblock->checksum_error;
> > @@ -2099,7 +2101,8 @@ static int scrub_checksum_super(struct scrub_block *sblock)
> >       crypto_shash_digest(shash, kaddr + BTRFS_CSUM_SIZE,
> >                       BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, calculated_csum);
> >
> > -     if (memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
> > +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
> > +         memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
> >               ++fail_cor;
> >
> >       return fail_cor + fail_gen;

Re: [PATCH RFC 3/7] fs/btrfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Qu Wenruo]

On 2022/10/17 16:43, Dmitry Vyukov wrote:
> On Fri, 14 Oct 2022 at 12:24, 'Qu Wenruo' via kasan-dev
> <kasan-dev@googlegroups.com> wrote:
>>
>> On 2022/10/14 16:48, Hrutvik Kanabar wrote:
>>> From: Hrutvik Kanabar <hrutvik@google.com>
>>>
>>> When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
>>> verification.
>>>
>>> Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
>>
>> I always want more fuzz for btrfs, so overall this is pretty good.
>>
>> But there are some comments related to free space cache part.
>>
>> Despite the details, I'm wondering would it be possible for your fuzzing
>> tool to do a better job at user space? Other than relying on loosen
>> checks from kernel?
>>
>> For example, implement a (mostly) read-only tool to do the following
>> workload:
>>
>> - Open the fs
>>     Including understand the checksum algo, how to re-generate the csum.
>>
>> - Read out the used space bitmap
>>     In btrfs case, it's going to read the extent tree, process the
>>     backrefs items.
>>
>> - Choose the victim sectors and corrupt them
>>     Obviously, vitims should be choosen from above used space bitmap.
>>
>> - Re-calculate the checksum for above corrupted sectors
>>     For btrfs, if it's a corrupted metadata, re-calculate the checksum.
>>
>> By this, we can avoid such change to kernel, and still get a much better
>> coverage.
>>
>> If you need some help on such user space tool, I'm pretty happy to
>> provide help.
>>
>>> ---
>>>    fs/btrfs/check-integrity.c  | 3 ++-
>>>    fs/btrfs/disk-io.c          | 6 ++++--
>>>    fs/btrfs/free-space-cache.c | 3 ++-
>>>    fs/btrfs/inode.c            | 3 ++-
>>>    fs/btrfs/scrub.c            | 9 ++++++---
>>>    5 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
>>> index 98c6e5feab19..eab82593a325 100644
>>> --- a/fs/btrfs/check-integrity.c
>>> +++ b/fs/btrfs/check-integrity.c
>>> @@ -1671,7 +1671,8 @@ static noinline_for_stack int btrfsic_test_for_metadata(
>>>                crypto_shash_update(shash, data, sublen);
>>>        }
>>>        crypto_shash_final(shash, csum);
>>> -     if (memcmp(csum, h->csum, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(csum, h->csum, fs_info->csum_size))
>>>                return 1;
>>>
>>>        return 0; /* is metadata */
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index a2da9313c694..7cd909d44b24 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -184,7 +184,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>>>        crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
>>>                            BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
>>>
>>> -     if (memcmp(disk_sb->csum, result, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(disk_sb->csum, result, fs_info->csum_size))
>>>                return 1;
>>>
>>>        return 0;
>>> @@ -494,7 +495,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>>>        header_csum = page_address(eb->pages[0]) +
>>>                get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
>>>
>>> -     if (memcmp(result, header_csum, csum_size) != 0) {
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(result, header_csum, csum_size) != 0) {
>>
>> I believe this is the main thing fuzzing would take advantage of.
>>
>> It would be much better if this is the only override...
>>
>>>                btrfs_warn_rl(fs_info,
>>>    "checksum verify failed on logical %llu mirror %u wanted " CSUM_FMT " found " CSUM_FMT " level %d",
>>>                              eb->start, eb->read_mirror,
>>> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
>>> index f4023651dd68..203c8a9076a6 100644
>>> --- a/fs/btrfs/free-space-cache.c
>>> +++ b/fs/btrfs/free-space-cache.c
>>> @@ -574,7 +574,8 @@ static int io_ctl_check_crc(struct btrfs_io_ctl *io_ctl, int index)
>>>        io_ctl_map_page(io_ctl, 0);
>>>        crc = btrfs_crc32c(crc, io_ctl->orig + offset, PAGE_SIZE - offset);
>>>        btrfs_crc32c_final(crc, (u8 *)&crc);
>>> -     if (val != crc) {
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         val != crc) {
>>
>> I'm already seeing this to cause problems, especially for btrfs.
>>
>> Btrfs has a very strong dependency on free space tracing, as all of our
>> metadata (and data by default) relies on COW to keep the fs consistent.
>>
>> I tried a lot of different methods in the past to make sure we won't
>> write into previously used space, but it's causing a lot of performance
>> impact.
>>
>> Unlike tree-checker, we can not easily got a centerlized space to handle
>> all the free space cross-check thing (thus it's only verified by things
>> like btrfs-check).
>>
>> Furthermore, even if you skip this override, with latest default
>> free-space-tree feature, free space info is stored in regular btrfs
>> metadata (tree blocks), with regular metadata checksum protection.
>>
>> Thus I'm pretty sure we will have tons of reports on this, and
>> unfortunately we can only go whac-a-mole way for it.
>
> Hi Qu,
>
> I don't fully understand what you mean. Could you please elaborate?
>
> Do you mean that btrfs uses this checksum check to detect blocks that
> were written to w/o updating the checksum?

I mean, btrfs uses this particular checksum for its (free) space cache,
and currently btrfs just trust the space cache completely to do COW.

This means, if we ignore the checksum for free space cache, we can
easily screw up the COW, e.g. allocate a range for the new metadata to
be written into.

But the truth is, that range is still being utilized by some other
metadata. Thus would completely break COW.


This is indeed a problem for btrfs, but it is not that easiy to fix,
since this involves cross-check 3 different data (free space cache for
free space, extent tree for used space, and the metadata itself).

Thus my concern is, disabling free space cache csum can easily lead to
various crashes, all related to broken COW, and we don't have a good
enough way to validate the result.

>
>
>
>
>>>                btrfs_err_rl(io_ctl->fs_info,
>>>                        "csum mismatch on free space cache");
>>>                io_ctl_unmap_page(io_ctl);
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index b0807c59e321..1a49d897b5c1 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -3434,7 +3434,8 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
>>>        crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
>>>        kunmap_local(kaddr);
>>>
>>> -     if (memcmp(csum, csum_expected, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(csum, csum_expected, fs_info->csum_size))
>>
>> This skips data csum check, I don't know how valueable it is, but this
>> should be harmless mostly.
>>
>> If we got reports related to this, it would be a nice surprise.
>>
>>>                return -EIO;
>>>        return 0;
>>>    }
>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>>> index f260c53829e5..a7607b492f47 100644
>>> --- a/fs/btrfs/scrub.c
>>> +++ b/fs/btrfs/scrub.c
>>> @@ -1997,7 +1997,8 @@ static int scrub_checksum_data(struct scrub_block *sblock)
>>>
>>>        crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
>>>
>>> -     if (memcmp(csum, sector->csum, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(csum, sector->csum, fs_info->csum_size))
>>
>> Same as data csum verification overide.
>> Not necessary/useful but good to have.
>>
>>>                sblock->checksum_error = 1;
>>>        return sblock->checksum_error;
>>>    }
>>> @@ -2062,7 +2063,8 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
>>>        }
>>>
>>>        crypto_shash_final(shash, calculated_csum);
>>> -     if (memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
>>
>> This is much less valueable, since it's only affecting scrub, and scrub
>> itself is already very little checking the content of metadata.
>
> Could you please elaborate here as well?

These checksum verification is only done in the scrub path (just as the
file name indicates).

> This is less valuable from what perspective?

It's just much harder to trigger, regular filesystem operations won't go
into scrub path.

Unless there is also a full ioctl fuzzing tests, after corrupting the image.

> The data loaded from disk can have any combination of
> (correct/incorrect metadata) x (correct/incorrect checksum).
> Correctness of metadata and checksum are effectively orthogonal,

Oh, I almost forgot another problem with the compile time csum
verification skip.

If we skip csum check completely, just like the patch, it may cause less
path coverage (this is very btrfs specific)

The problem is, btrfs has some repair path (scrub, and read-time), which
requires to have a checksum mismatch (and a good copy with good checksum).

Thus if we ignore csum completely, the repair path will never be covered
(as we treat them all as csum match).

Thanks,
Qu

> right?
>
>
>
>> Thanks,
>> Qu
>>
>>>                sblock->checksum_error = 1;
>>>
>>>        return sblock->header_error || sblock->checksum_error;
>>> @@ -2099,7 +2101,8 @@ static int scrub_checksum_super(struct scrub_block *sblock)
>>>        crypto_shash_digest(shash, kaddr + BTRFS_CSUM_SIZE,
>>>                        BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, calculated_csum);
>>>
>>> -     if (memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
>>>                ++fail_cor;
>>>
>>>        return fail_cor + fail_gen;

[PATCH RFC 4/7] fs/exfat: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
verification.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/exfat/nls.c   | 3 ++-
 fs/exfat/super.c | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
index 705710f93e2d..f0f92eaf6ccc 100644
--- a/fs/exfat/nls.c
+++ b/fs/exfat/nls.c
@@ -696,7 +696,8 @@ static int exfat_load_upcase_table(struct super_block *sb,
 		brelse(bh);
 	}
 
-	if (index >= 0xFFFF && utbl_checksum == chksum)
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) ||
+	    (index >= 0xFFFF && utbl_checksum == chksum))
 		return 0;
 
 	exfat_err(sb, "failed to load upcase table (idx : 0x%08x, chksum : 0x%08x, utbl_chksum : 0x%08x)",
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 35f0305cd493..7418858792b3 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -564,6 +564,9 @@ static int exfat_verify_boot_region(struct super_block *sb)
 	if (!bh)
 		return -EIO;
 
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION))
+		return 0;
+
 	for (i = 0; i < sb->s_blocksize; i += sizeof(u32)) {
 		p_chksum = (__le32 *)&bh->b_data[i];
 		if (le32_to_cpu(*p_chksum) != chksum) {
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH RFC 5/7] fs/xfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When `DISABLE_FS_CSUM_VERIFICATION` is enabled, return truthy value for
`xfs_verify_cksum`, which is the key function implementing checksum
verification for XFS.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/xfs/libxfs/xfs_cksum.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_cksum.h b/fs/xfs/libxfs/xfs_cksum.h
index 999a290cfd72..ba55b1afa382 100644
--- a/fs/xfs/libxfs/xfs_cksum.h
+++ b/fs/xfs/libxfs/xfs_cksum.h
@@ -76,7 +76,10 @@ xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
 {
 	uint32_t crc = xfs_start_cksum_safe(buffer, length, cksum_offset);
 
-	return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION))
+		return 1;
+	else
+		return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
 }
 
 #endif /* _XFS_CKSUM_H */
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH RFC 5/7] fs/xfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Darrick J. Wong]

On Fri, Oct 14, 2022 at 08:48:35AM +0000, Hrutvik Kanabar wrote:
> From: Hrutvik Kanabar <hrutvik@google.com>
> 
> When `DISABLE_FS_CSUM_VERIFICATION` is enabled, return truthy value for
> `xfs_verify_cksum`, which is the key function implementing checksum
> verification for XFS.
> 
> Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>

NAK, we're not going to break XFS for the sake of automated fuzz tools.

You'll have to adapt your fuzzing tools to rewrite the block header
checksums, like the existing xfs fuzz testing framework does.  See
the xfs_db 'fuzz -d' command and the relevant fstests.

--D

> ---
>  fs/xfs/libxfs/xfs_cksum.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_cksum.h b/fs/xfs/libxfs/xfs_cksum.h
> index 999a290cfd72..ba55b1afa382 100644
> --- a/fs/xfs/libxfs/xfs_cksum.h
> +++ b/fs/xfs/libxfs/xfs_cksum.h
> @@ -76,7 +76,10 @@ xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
>  {
>  	uint32_t crc = xfs_start_cksum_safe(buffer, length, cksum_offset);
>  
> -	return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
> +	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION))
> +		return 1;
> +	else
> +		return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
>  }
>  
>  #endif /* _XFS_CKSUM_H */
> -- 
> 2.38.0.413.g74048e4d9e-goog
> 

Re: [PATCH RFC 5/7] fs/xfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Dmitry Vyukov]

On Fri, 14 Oct 2022 at 17:44, Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Fri, Oct 14, 2022 at 08:48:35AM +0000, Hrutvik Kanabar wrote:
> > From: Hrutvik Kanabar <hrutvik@google.com>
> >
> > When `DISABLE_FS_CSUM_VERIFICATION` is enabled, return truthy value for
> > `xfs_verify_cksum`, which is the key function implementing checksum
> > verification for XFS.
> >
> > Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
>
> NAK, we're not going to break XFS for the sake of automated fuzz tools.

Hi Darrick,

What do you mean by "break"? If this config is not enabled the
behavior is not affected as far as I see.

> You'll have to adapt your fuzzing tools to rewrite the block header
> checksums, like the existing xfs fuzz testing framework does.  See
> the xfs_db 'fuzz -d' command and the relevant fstests.
>
> --D
>
> > ---
> >  fs/xfs/libxfs/xfs_cksum.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_cksum.h b/fs/xfs/libxfs/xfs_cksum.h
> > index 999a290cfd72..ba55b1afa382 100644
> > --- a/fs/xfs/libxfs/xfs_cksum.h
> > +++ b/fs/xfs/libxfs/xfs_cksum.h
> > @@ -76,7 +76,10 @@ xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
> >  {
> >       uint32_t crc = xfs_start_cksum_safe(buffer, length, cksum_offset);
> >
> > -     return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
> > +     if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION))
> > +             return 1;
> > +     else
> > +             return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
> >  }
> >
> >  #endif /* _XFS_CKSUM_H */
> > --
> > 2.38.0.413.g74048e4d9e-goog
> >
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/Y0mD0LcNvu%2BQTlQ9%40magnolia.

[PATCH RFC 6/7] fs/ntfs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
verification in `is_boot_sector_ntfs`.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/ntfs/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 001f4e053c85..428c65ce9a80 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -582,7 +582,8 @@ static bool is_boot_sector_ntfs(const struct super_block *sb,
 
 		for (i = 0, u = (le32*)b; u < (le32*)(&b->checksum); ++u)
 			i += le32_to_cpup(u);
-		if (le32_to_cpu(b->checksum) != i)
+		if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+		    le32_to_cpu(b->checksum) != i)
 			ntfs_warning(sb, "Invalid boot sector checksum.");
 	}
 	/* Check OEMidentifier is "NTFS    " */
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH RFC 7/7] fs/f2fs: support `DISABLE_FS_CSUM_VERIFICATION` config option

[Hrutvik Kanabar]

From: Hrutvik Kanabar <hrutvik@google.com>

When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
verification.

Signed-off-by: Hrutvik Kanabar <hrutvik@google.com>
---
 fs/f2fs/checkpoint.c | 3 ++-
 fs/f2fs/compress.c   | 3 ++-
 fs/f2fs/f2fs.h       | 2 ++
 fs/f2fs/inode.c      | 3 +++
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 0c82dae082aa..cc5043fbffcb 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -864,7 +864,8 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
 	}
 
 	crc = f2fs_checkpoint_chksum(sbi, *cp_block);
-	if (crc != cur_cp_crc(*cp_block)) {
+	if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+	    crc != cur_cp_crc(*cp_block)) {
 		f2fs_put_page(*cp_page, 1);
 		f2fs_warn(sbi, "invalid crc value");
 		return -EINVAL;
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index d315c2de136f..d0bce92dbf38 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -772,7 +772,8 @@ void f2fs_decompress_cluster(struct decompress_io_ctx *dic, bool in_task)
 		u32 provided = le32_to_cpu(dic->cbuf->chksum);
 		u32 calculated = f2fs_crc32(sbi, dic->cbuf->cdata, dic->clen);
 
-		if (provided != calculated) {
+		if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
+		    provided != calculated) {
 			if (!is_inode_flag_set(dic->inode, FI_COMPRESS_CORRUPT)) {
 				set_inode_flag(dic->inode, FI_COMPRESS_CORRUPT);
 				printk_ratelimited(
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index e6355a5683b7..b27f1ec9b49f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1976,6 +1976,8 @@ static inline u32 f2fs_crc32(struct f2fs_sb_info *sbi, const void *address,
 static inline bool f2fs_crc_valid(struct f2fs_sb_info *sbi, __u32 blk_crc,
 				  void *buf, size_t buf_size)
 {
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION))
+		return true;
 	return f2fs_crc32(sbi, buf, buf_size) == blk_crc;
 }
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 9f0d3864d9f1..239bb08e45b1 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -181,6 +181,9 @@ bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
 #endif
 		return true;
 
+	if (IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION))
+		return true;
+
 	ri = &F2FS_NODE(page)->i;
 	provided = le32_to_cpu(ri->i_inode_checksum);
 	calculated = f2fs_inode_chksum(sbi, page);
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH RFC 0/7] fs: Debug config option to disable filesystem checksum verification for fuzzing

[David Sterba]

On Fri, Oct 14, 2022 at 08:48:30AM +0000, Hrutvik Kanabar wrote:
> From: Hrutvik Kanabar <hrutvik@google.com>
> 
> Fuzzing is a proven technique to discover exploitable bugs in the Linux
> kernel. But fuzzing filesystems is tricky: highly structured disk images
> use redundant checksums to verify data integrity. Therefore,
> randomly-mutated images are quickly rejected as corrupt, testing only
> error-handling code effectively.
> 
> The Janus [1] and Hydra [2] projects probe filesystem code deeply by
> correcting checksums after mutation. But their ad-hoc
> checksum-correcting code supports only a few filesystems, and it is
> difficult to support new ones - requiring significant duplication of
> filesystem logic which must also be kept in sync with upstream changes.
> Corrected checksums cannot be guaranteed to be valid, and reusing this
> code across different fuzzing frameworks is non-trivial.
> 
> Instead, this RFC suggests a config option:
> `DISABLE_FS_CSUM_VERIFICATION`. When it is enabled, all filesystems
> should bypass redundant checksum verification, proceeding as if
> checksums are valid. Setting of checksums should be unaffected. Mutated
> images will no longer be rejected due to invalid checksums, allowing
> testing of deeper code paths. Though some filesystems implement their
> own flags to disable some checksums, this option should instead disable
> all checksums for all filesystems uniformly. Critically, any bugs found
> remain reproducible on production systems: redundant checksums in
> mutated images can be fixed up to satisfy verification.
> 
> The patches below suggest a potential implementation for a few
> filesystems, though we may have missed some checksums. The option
> requires `DEBUG_KERNEL` and is not intended for production systems.
> 
> The first user of the option would be syzbot. We ran preliminary local
> syzkaller tests to compare behaviour with and without these patches.
> With the patches, we found a 19% increase in coverage, as well as many
> new crash types and increases in the total number of crashes:

I think the build-time option inflexible, but I see the point when
you're testing several filesystems that it's one place to set up the
environment. Alternatively I suggest to add sysfs knob available in
debuging builds to enable/disable checksum verification per filesystem.

As this may not fit to other filesystems I don't suggest to do that for
all but I am willing to do that for btrfs, with eventual extension to
the config option you propose. The increased fuzzing coverage would be
good to have.

Re: [PATCH RFC 0/7] fs: Debug config option to disable filesystem checksum verification for fuzzing

[Dmitry Vyukov]

On Fri, 14 Oct 2022 at 11:15, David Sterba <dsterba@suse.cz> wrote:
>
> On Fri, Oct 14, 2022 at 08:48:30AM +0000, Hrutvik Kanabar wrote:
> > From: Hrutvik Kanabar <hrutvik@google.com>
> >
> > Fuzzing is a proven technique to discover exploitable bugs in the Linux
> > kernel. But fuzzing filesystems is tricky: highly structured disk images
> > use redundant checksums to verify data integrity. Therefore,
> > randomly-mutated images are quickly rejected as corrupt, testing only
> > error-handling code effectively.
> >
> > The Janus [1] and Hydra [2] projects probe filesystem code deeply by
> > correcting checksums after mutation. But their ad-hoc
> > checksum-correcting code supports only a few filesystems, and it is
> > difficult to support new ones - requiring significant duplication of
> > filesystem logic which must also be kept in sync with upstream changes.
> > Corrected checksums cannot be guaranteed to be valid, and reusing this
> > code across different fuzzing frameworks is non-trivial.
> >
> > Instead, this RFC suggests a config option:
> > `DISABLE_FS_CSUM_VERIFICATION`. When it is enabled, all filesystems
> > should bypass redundant checksum verification, proceeding as if
> > checksums are valid. Setting of checksums should be unaffected. Mutated
> > images will no longer be rejected due to invalid checksums, allowing
> > testing of deeper code paths. Though some filesystems implement their
> > own flags to disable some checksums, this option should instead disable
> > all checksums for all filesystems uniformly. Critically, any bugs found
> > remain reproducible on production systems: redundant checksums in
> > mutated images can be fixed up to satisfy verification.
> >
> > The patches below suggest a potential implementation for a few
> > filesystems, though we may have missed some checksums. The option
> > requires `DEBUG_KERNEL` and is not intended for production systems.
> >
> > The first user of the option would be syzbot. We ran preliminary local
> > syzkaller tests to compare behaviour with and without these patches.
> > With the patches, we found a 19% increase in coverage, as well as many
> > new crash types and increases in the total number of crashes:
>
> I think the build-time option inflexible, but I see the point when
> you're testing several filesystems that it's one place to set up the
> environment. Alternatively I suggest to add sysfs knob available in
> debuging builds to enable/disable checksum verification per filesystem.

Hi David,

What usage scenarios do you have in mind for runtime changing of this option?
I see this option intended only for very narrow use cases which
require a specially built kernel in a number of other ways (lots of
which are not tunable at runtime, e.g. debugging configs).

> As this may not fit to other filesystems I don't suggest to do that for
> all but I am willing to do that for btrfs, with eventual extension to
> the config option you propose. The increased fuzzing coverage would be
> good to have.

Re: [PATCH RFC 0/7] fs: Debug config option to disable filesystem checksum verification for fuzzing

[David Sterba]

On Mon, Oct 17, 2022 at 10:31:03AM +0200, Dmitry Vyukov wrote:
> On Fri, 14 Oct 2022 at 11:15, David Sterba <dsterba@suse.cz> wrote:
> > On Fri, Oct 14, 2022 at 08:48:30AM +0000, Hrutvik Kanabar wrote:
> > > From: Hrutvik Kanabar <hrutvik@google.com>
> > I think the build-time option inflexible, but I see the point when
> > you're testing several filesystems that it's one place to set up the
> > environment. Alternatively I suggest to add sysfs knob available in
> > debuging builds to enable/disable checksum verification per filesystem.
> 
> What usage scenarios do you have in mind for runtime changing of this option?
> I see this option intended only for very narrow use cases which
> require a specially built kernel in a number of other ways (lots of
> which are not tunable at runtime, e.g. debugging configs).

For my own development and testing usecase I'd like to build the kernel
from the same config all the time, then start a VM and run random tests
that do not skip the checksum verification. Then as the last also run
fuzzing with checksums skipped. The debugging (lockdep, various sanity
checks, ...) config options are enabled. We both have a narrow usecase,
what I'm suggesting is a common way to enable them.