Re: [PATCH 0/2] Fix out-of-bound access if pagecache of udf device is corrupted

Re: [PATCH 0/2] Fix out-of-bound access if pagecache of udf device is corrupted

[Jan Kara]

On Tue 13-06-23 11:22:52, Wenchao Hao wrote:
> Following steps would cause out-of-bound access and even cause kernel
> panic when using udf:
> 
> dd if=/dev/zero of=udf.img bs=1M count=512
> mkfs.udf udf.img
> mount -o loop -t udf udf.img /mnt
> dd if=/dev/random of=/dev/loop0 bs=512 count=1 seek=128
> umount /mnt
> 
> [if /mnt is mounted on /dev/loop0]
> 
> It is because we did not check if udf_sb_info->s_lvid_bh is valid in
> udf_sb_lvidiu().
> 
> Although it's illegal to write backend device since filesystem has been
> mounted, but we should avoid kernel panic if it happened.

No, it is perfectly valid to crash the kernel if someone writes the buffer
cache of the device while the device is mounted (which your example above
does). There is no practical protection against this because someone could
overwrite the buffer just after the moment you verify its validity. The
only protection would be to lock the buffer for each access and fully
verify validity of the data after each locking but the performance and
maintenance overhead of this is too high to justify. So I'm sorry but I
will not take any patches that try to "fix" situations when someone writes
buffer cache while the filesystem is mounted.

I guess your work is motivated by some syzbot reproducer which was doing
this. Let me work on a kernel option which syzbot can use to not report
these issues.


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] Fix out-of-bound access if pagecache of udf device is corrupted

[haowenchao (C)]

On 2023/6/12 22:40, Jan Kara wrote:
> On Tue 13-06-23 11:22:52, Wenchao Hao wrote:
>> Following steps would cause out-of-bound access and even cause kernel
>> panic when using udf:
>>
>> dd if=/dev/zero of=udf.img bs=1M count=512
>> mkfs.udf udf.img
>> mount -o loop -t udf udf.img /mnt
>> dd if=/dev/random of=/dev/loop0 bs=512 count=1 seek=128
>> umount /mnt
>>
>> [if /mnt is mounted on /dev/loop0]
>>
>> It is because we did not check if udf_sb_info->s_lvid_bh is valid in
>> udf_sb_lvidiu().
>>
>> Although it's illegal to write backend device since filesystem has been
>> mounted, but we should avoid kernel panic if it happened.
> 
> No, it is perfectly valid to crash the kernel if someone writes the buffer
> cache of the device while the device is mounted (which your example above
> does). There is no practical protection against this because someone could
> overwrite the buffer just after the moment you verify its validity. The
> only protection would be to lock the buffer for each access and fully
> verify validity of the data after each locking but the performance and
> maintenance overhead of this is too high to justify. So I'm sorry but I
> will not take any patches that try to "fix" situations when someone writes
> buffer cache while the filesystem is mounted.
> 
> I guess your work is motivated by some syzbot reproducer which was doing
> this. Let me work on a kernel option which syzbot can use to not report
> these issues.
> 
> 
> 								Honza

Yes, the issue is discovered by syzbot. Looking forward you patches.

[PATCH 0/2] Fix out-of-bound access if pagecache of udf device is corrupted

[Wenchao Hao]

Following steps would cause out-of-bound access and even cause kernel
panic when using udf:

dd if=/dev/zero of=udf.img bs=1M count=512
mkfs.udf udf.img
mount -o loop -t udf udf.img /mnt
dd if=/dev/random of=/dev/loop0 bs=512 count=1 seek=128
umount /mnt

[if /mnt is mounted on /dev/loop0]

It is because we did not check if udf_sb_info->s_lvid_bh is valid in
udf_sb_lvidiu().

Although it's illegal to write backend device since filesystem has been
mounted, but we should avoid kernel panic if it happened.

The first patch add a helper function to check if the data is valid.
The second patch just call the helper function, if check failed, return
NULL from udf_sb_lvidiu()

Wenchao Hao (2):
  udf: add helper function udf_check_tagged_bh to check tagged page
  udf:check if buffer head's data when getting lvidiu

 fs/udf/misc.c    | 60 ++++++++++++++++++++++++++++--------------------
 fs/udf/super.c   |  2 ++
 fs/udf/udfdecl.h |  1 +
 3 files changed, 38 insertions(+), 25 deletions(-)

-- 
2.35.3

[PATCH 1/2] udf: add helper function udf_check_tagged_bh to check tagged page

[Wenchao Hao]

This helper function is used to check if a buffer head's data is valid
and would be called in future.

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 fs/udf/misc.c    | 60 ++++++++++++++++++++++++++++--------------------
 fs/udf/udfdecl.h |  1 +
 2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/fs/udf/misc.c b/fs/udf/misc.c
index 3777468d06ce..b20b53fc8d41 100644
--- a/fs/udf/misc.c
+++ b/fs/udf/misc.c
@@ -179,6 +179,40 @@ struct genericFormat *udf_get_extendedattr(struct inode *inode, uint32_t type,
 	return NULL;
 }
 
+bool udf_check_tagged_bh(struct super_block *sb, struct buffer_head *bh)
+{
+	u8 checksum;
+	struct tag *tag_p = (struct tag *)(bh->b_data);
+
+	/* Verify the tag checksum */
+	checksum = udf_tag_checksum(tag_p);
+	if (checksum != tag_p->tagChecksum) {
+		udf_err(sb, "tag checksum failed, block %llu: 0x%02x != 0x%02x\n",
+			bh->b_blocknr, checksum, tag_p->tagChecksum);
+		return false;
+	}
+
+	/* Verify the tag version */
+	if (tag_p->descVersion != cpu_to_le16(0x0002U) &&
+	    tag_p->descVersion != cpu_to_le16(0x0003U)) {
+		udf_err(sb, "tag version 0x%04x != 0x0002 || 0x0003, block %llu\n",
+			le16_to_cpu(tag_p->descVersion), bh->b_blocknr);
+		return false;
+	}
+
+	/* Verify the descriptor CRC */
+	if (le16_to_cpu(tag_p->descCRCLength) + sizeof(struct tag) > sb->s_blocksize ||
+	    le16_to_cpu(tag_p->descCRC) == crc_itu_t(0,
+					bh->b_data + sizeof(struct tag),
+					le16_to_cpu(tag_p->descCRCLength)))
+		return true;
+
+	udf_debug("Crc failure block %llu: crc = %u, crclen = %u\n", bh->b_blocknr,
+		  le16_to_cpu(tag_p->descCRC),
+		  le16_to_cpu(tag_p->descCRCLength));
+	return false;
+}
+
 /*
  * udf_read_tagged
  *
@@ -194,7 +228,6 @@ struct buffer_head *udf_read_tagged(struct super_block *sb, uint32_t block,
 {
 	struct tag *tag_p;
 	struct buffer_head *bh = NULL;
-	u8 checksum;
 
 	/* Read the block */
 	if (block == 0xFFFFFFFF)
@@ -217,32 +250,9 @@ struct buffer_head *udf_read_tagged(struct super_block *sb, uint32_t block,
 		goto error_out;
 	}
 
-	/* Verify the tag checksum */
-	checksum = udf_tag_checksum(tag_p);
-	if (checksum != tag_p->tagChecksum) {
-		udf_err(sb, "tag checksum failed, block %u: 0x%02x != 0x%02x\n",
-			block, checksum, tag_p->tagChecksum);
-		goto error_out;
-	}
-
-	/* Verify the tag version */
-	if (tag_p->descVersion != cpu_to_le16(0x0002U) &&
-	    tag_p->descVersion != cpu_to_le16(0x0003U)) {
-		udf_err(sb, "tag version 0x%04x != 0x0002 || 0x0003, block %u\n",
-			le16_to_cpu(tag_p->descVersion), block);
-		goto error_out;
-	}
-
-	/* Verify the descriptor CRC */
-	if (le16_to_cpu(tag_p->descCRCLength) + sizeof(struct tag) > sb->s_blocksize ||
-	    le16_to_cpu(tag_p->descCRC) == crc_itu_t(0,
-					bh->b_data + sizeof(struct tag),
-					le16_to_cpu(tag_p->descCRCLength)))
+	if (udf_check_tagged_bh(sb, bh))
 		return bh;
 
-	udf_debug("Crc failure block %u: crc = %u, crclen = %u\n", block,
-		  le16_to_cpu(tag_p->descCRC),
-		  le16_to_cpu(tag_p->descCRCLength));
 error_out:
 	brelse(bh);
 	return NULL;
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 88692512a466..fb269752b9c6 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -180,6 +180,7 @@ extern struct genericFormat *udf_add_extendedattr(struct inode *, uint32_t,
 						  uint32_t, uint8_t);
 extern struct genericFormat *udf_get_extendedattr(struct inode *, uint32_t,
 						  uint8_t);
+extern bool udf_check_tagged_bh(struct super_block *sb, struct buffer_head *bh);
 extern struct buffer_head *udf_read_tagged(struct super_block *, uint32_t,
 					   uint32_t, uint16_t *);
 extern struct buffer_head *udf_read_ptagged(struct super_block *,
-- 
2.35.3

[PATCH 2/2] udf:check if buffer head's data when getting lvidiu

[Wenchao Hao]

We can not always assume udf_sb_info->s_lvid_bh's data is valid. If the
data is corrupted, we would get an incorrect offset and cause the
following code access an illegal address.

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 fs/udf/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 6304e3c5c3d9..71481b60c871 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -114,6 +114,8 @@ struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct super_block *sb)
 
 	if (!UDF_SB(sb)->s_lvid_bh)
 		return NULL;
+	if (!udf_check_tagged_bh(sb, UDF_SB(sb)->s_lvid_bh))
+		return NULL;
 	lvid = (struct logicalVolIntegrityDesc *)UDF_SB(sb)->s_lvid_bh->b_data;
 	partnum = le32_to_cpu(lvid->numOfPartitions);
 	/* The offset is to skip freeSpaceTable and sizeTable arrays */
-- 
2.35.3