fs/nilfs2: Fix potential underflow in call to crc32_le

PATCH Superseded Torsten Hilbrich 3 participants 8 messages Jun 17, 2016 – Jun 20, 2016 5 messages not in the archive NILFS2 filesystem

lore

Share & tools

lore

Torsten Hilbrich

author

2016-06-17 08:06 UTC #0 lore raw

PATCH 1/1 fs/nilfs2: Fix potential underflow in call to crc32_le

The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.

Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4 and smaller than the remaining space in the block where the superblock is stored (BLOCK_SIZE - sumoff - 4).

This fixes a problem where the accidential mounting of an encrypted volume caused a kernel panic. It had the correct magic value 0x3434 at offset 0x406 by chance and the following 2 bytes were 0x01, 0x00 (interpreted as s_bytes value with value 1).

Signed-off-by Torsten Hilbrich <[email protected]>

Tested-by Torsten Hilbrich <[email protected]>

---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/nilfs2/the_nilfs.c +4 -3

index 69bd801..21f6b23 100644
@@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	size_t crc_offset = sumoff + 4;
 	u32 crc;
 
 	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
 		return 0;
 	bytes = le16_to_cpu(sbp->s_bytes);
-	if (bytes > BLOCK_SIZE)
+	if (bytes < crc_offset || bytes > BLOCK_SIZE - crc_offset)
 		return 0;
 	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
 		       sumoff);
 	crc = crc32_le(crc, sum, 4);
-	crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
-		       bytes - sumoff - 4);
+	crc = crc32_le(crc, (unsigned char *)sbp + crc_offset,
+		       bytes - crc_offset);
 	return crc == le32_to_cpu(sbp->s_sum);
 }

Signature · 5 lines

2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Viacheslav Dubeyko

2016-06-17 16:51 UTC #2 lore raw

Quoted 25 lines · Torsten Hilbrich

The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.
Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4 and smaller than the remaining space in the block where the superblock is stored (BLOCK_SIZE - sumoff - 4).
This fixes a problem where the accidential mounting of an encrypted volume caused a kernel panic. It had the correct magic value 0x3434 at offset 0x406 by chance and the following 2 bytes were 0x01, 0x00 (interpreted as s_bytes value with value 1).
Signed-off-by Torsten Hilbrich <[email protected]>
Tested-by Torsten Hilbrich <[email protected]>
---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
fs/nilfs2/the_nilfs.c +0 -0
index 69bd801..21f6b23 100644

fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 441

Quoted 4 lines

fs/nilfs2/the_nilfs.c +1 -0

@@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	size_t crc_offset = sumoff + 4;

Hard-coded value looks very weird and not understandable, usually. Could you change hard-coded value on some declared constant with reasonable name or on sizeof(something)?

Thanks,
Vyacheslav Dubeyko.

Quoted 8 lines

fs/nilfs2/the_nilfs.c +1 -1

 	u32 crc;
 
 	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
 		return 0;
 	bytes = le16_to_cpu(sbp->s_bytes);
-	if (bytes > BLOCK_SIZE)
+	if (bytes < crc_offset || bytes > BLOCK_SIZE - crc_offset)
 		return 0;

fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 456

Quoted 10 lines

fs/nilfs2/the_nilfs.c +2 -2

 	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
 		       sumoff);
 	crc = crc32_le(crc, sum, 4);
-	crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
-		       bytes - sumoff - 4);
+	crc = crc32_le(crc, (unsigned char *)sbp + crc_offset,
+		       bytes - crc_offset);
 	return crc == le32_to_cpu(sbp->s_sum);
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Ryusuke Konishi

2016-06-17 18:25 UTC #3 lore raw

In reply to a message not in the archive

Hi,

I have a few comments. Please see the inline comments below.

Quoted 10 lines · Torsten Hilbrich

The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.
Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4 and smaller than the remaining space in the block where the superblock is stored (BLOCK_SIZE - sumoff - 4).

Quoted 4 lines

This fixes a problem where the accidential mounting of an encrypted volume caused a kernel panic. It had the correct magic value 0x3434 at offset 0x406 by chance and the following 2 bytes were 0x01, 0x00 (interpreted as s_bytes value with value 1).

Could you improve the description on the problem a bit ?

The true issue looks a "memory access overrun" on the superblock buffer that can occur when "s_bytes - sumoff - 4" underflows and crc32_le() receives an uninteded large byte count.

The reason why a "kernel panic" occurs, is unclear in the comment.

Even if a non-nilfs volume is mistakenly mounted as a nilfs volume, it usually will return an error. Kernel panic should never occur just by an accidental mount. If it happens except the overrun issue, it is another bug.

Quoted 12 lines

Signed-off-by Torsten Hilbrich <[email protected]>
Tested-by Torsten Hilbrich <[email protected]>
---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
fs/nilfs2/the_nilfs.c +0 -0
index 69bd801..21f6b23 100644

fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 441

Quoted 4 lines

fs/nilfs2/the_nilfs.c +1 -0

@@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	size_t crc_offset = sumoff + 4;

The name "crc_offset" is confusing. This just means the offset of the latter part of the region that the crc function will verify. In addition, it should be defined with a "const" qualifier.

fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp)

Quoted 8 lines

fs/nilfs2/the_nilfs.c +1 -1

 	u32 crc;
 
 	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
 		return 0;
 	bytes = le16_to_cpu(sbp->s_bytes);
-	if (bytes > BLOCK_SIZE)
 
+	if (bytes < crc_offset || bytes > BLOCK_SIZE - crc_offset)

The latter part of this test should be "bytes > BLOCK_SIZE". Why do you subtract crc_offset here ?

Regards,
Ryusuke Konishi

Quoted 17 lines

fs/nilfs2/the_nilfs.c +2 -2

 		return 0;
 	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
 		       sumoff);
 	crc = crc32_le(crc, sum, 4);
-	crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
-		       bytes - sumoff - 4);
+	crc = crc32_le(crc, (unsigned char *)sbp + crc_offset,
+		       bytes - crc_offset);
 	return crc == le32_to_cpu(sbp->s_sum);
 }

Signature · 6 lines

-- 
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20160618.032507.1597365191396955107.konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

Torsten Hilbrich

author

2016-06-20 04:41 UTC #5 lore raw

Quoted 26 lines · Ryusuke Konishi

Hi,
I have a few comments. Please see the inline comments below.
Quoted 10 lines · Torsten Hilbrich
The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.
Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4 and smaller than the remaining space in the block where the superblock is stored (BLOCK_SIZE - sumoff - 4).
Quoted 4 lines
This fixes a problem where the accidential mounting of an encrypted volume caused a kernel panic. It had the correct magic value 0x3434 at offset 0x406 by chance and the following 2 bytes were 0x01, 0x00 (interpreted as s_bytes value with value 1).
Could you improve the description on the problem a bit ?
The true issue looks a "memory access overrun" on the superblock buffer that can occur when "s_bytes - sumoff - 4" underflows and crc32_le() receives an uninteded large byte count.

Quoted 6 lines

The reason why a "kernel panic" occurs, is unclear in the comment.
Even if a non-nilfs volume is mistakenly mounted as a nilfs volume, it usually will return an error. Kernel panic should never occur just by an accidental mount. If it happens except the overrun issue, it is another bug.

You are right, I only got the kernel panic when first testing on a grsecurity kernel. I repeated the test and there just a BUG on the underflow/overrun is reported. I have attached the kernel logs of this test run to this mail (nilfs.bug).

Quoted 8 lines

Quoted 8 lines
Signed-off-by Torsten Hilbrich <[email protected]>
Tested-by Torsten Hilbrich <[email protected]>
---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Quoted 13 lines

fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 441
Quoted 9 lines
fs/nilfs2/the_nilfs.c +1 -0
index 69bd801..21f6b23 100644
@@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	size_t crc_offset = sumoff + 4;
The name "crc_offset" is confusing. This just means the offset of the latter part of the region that the crc function will verify. In addition, it should be defined with a "const" qualifier.

I will rename it to crc_start.

Quoted 8 lines

Quoted 7 lines

fs/nilfs2/the_nilfs.c +0 -1

 	u32 crc;
 
 	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
 		return 0;
 	bytes = le16_to_cpu(sbp->s_bytes);
-	if (bytes > BLOCK_SIZE)

Quoted 4 lines

fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 447
Quoted 1 line
fs/nilfs2/the_nilfs.c +1 -0
+	if (bytes < crc_offset || bytes > BLOCK_SIZE - crc_offset)
The latter part of this test should be "bytes > BLOCK_SIZE". Why do you subtract crc_offset here ?

I made here some wrong assumption about the memory layout and the meaning of bytes. bytes is the end of the area which is to be crc32-checked so comparing against BLOCK_SIZE (which should be the maximum size of the super block) is okay.

Quoted 15 lines

Regards,
Ryusuke Konishi

Quoted 11 lines

fs/nilfs2/the_nilfs.c +2 -2

 		return 0;
 	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
 		       sumoff);
 	crc = crc32_le(crc, sum, 4);
-	crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
-		       bytes - sumoff - 4);
+	crc = crc32_le(crc, (unsigned char *)sbp + crc_offset,
+		       bytes - crc_offset);
 	return crc == le32_to_cpu(sbp->s_sum);
 }

Quoted 3 lines

Quoted 3 lines
Signature · 2 lines
2.1.4
--

I will sent an updated patch.

Torsten

[201699.185465] BUG: unable to handle kernel paging request at ffff88021e600000
[201699.186111] IP: [<ffffffff814083c6>] crc32_le+0x36/0x100
[201699.186724] PGD 21ff067 PUD 2202067 PMD 0 
[201699.187333] Oops: 0000 [#1] SMP 
[201699.187936] Modules linked in: nilfs2 ec_sys nls_iso8859_1 uas usb_storage usbhid hid msr algif_skcipher af_alg ctr ccm bnep snd_hrtimer pci_stub vboxpci(OE) vboxnetadp(OE) vboxnetflt(OE) bluetooth vboxdrv(OE) binfmt_misc drbg ansi_cprng dm_crypt uvcvideo intel_rapl x86_pkg_temp_thermal intel_powerclamp videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 coretemp kvm_intel kvm videobuf2_core irqbypass v4l2_common crct10dif_pclmul crc32_pclmul videodev media aesni_intel arc4 cdc_mbim aes_x86_64 lrw iwldvm gf128mul glue_helper snd_hda_codec_hdmi ablk_helper mac80211 cdc_ncm cryptd usbnet mii cdc_acm cdc_wdm snd_hda_codec_conexant snd_hda_codec_generic input_leds snd_hda_intel iwlwifi snd_hda_codec serio_raw snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event thinkpad_acpi snd_rawmidi cfg80211
[201699.190960]  snd_seq nvram snd_seq_device shpchp snd_timer lpc_ich snd mei_me mei soundcore mac_hid cuse parport_pc ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper psmouse ahci sdhci_pci syscopyarea sysfillrect sysimgblt libahci fb_sys_fops sdhci drm e1000e ptp pps_core wmi fjes video
[201699.192637] CPU: 2 PID: 17038 Comm: mount Tainted: G           OE   4.4.0-24-generic #43-Ubuntu
[201699.193447] Hardware name: LENOVO 4236NGG/4236NGG, BIOS 83ET76WW (1.46 ) 07/05/2013
[201699.194265] task: ffff8801ea069b80 ti: ffff8801216d8000 task.ti: ffff8801216d8000
[201699.195074] RIP: 0010:[<ffffffff814083c6>]  [<ffffffff814083c6>] crc32_le+0x36/0x100
[201699.195886] RSP: 0018:ffff8801216dbc50  EFLAGS: 00010296
[201699.196698] RAX: 0000000000000000 RBX: 000000000000002b RCX: 0000000000000065
[201699.197499] RDX: ffffffffffffffe8 RSI: ffff88021e600000 RDI: 00000000dd21f2f1
[201699.198320] RBP: ffff8801216dbc58 R08: ffff8801527b33f8 R09: ffff8801527b3410
[201699.199101] R10: 0000000000000005 R11: 00000000000000b4 R12: 0000000000000001
[201699.199889] R13: 0000000000000400 R14: ffff8801216dbce8 R15: 00000000000ff000
[201699.200669] FS:  00007efc89607840(0000) GS:ffff88021e280000(0000) knlGS:0000000000000000
[201699.201456] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[201699.202253] CR2: ffff88021e600000 CR3: 000000019e010000 CR4: 00000000000406e0
[201699.203040] Stack:
[201699.203811]  ffff8801527b3400 ffff8801216dbc78 ffffffffc0907492 ffff88020ff64c00
[201699.204608]  ffff8801bda5d800 ffff8801216dbcd8 ffffffffc09075e2 ffffffff812479fd
[201699.205393]  ffff880161944000 ffff8801527b3400 ffff8801bda5d800 0000000061c2cd82
[201699.206202] Call Trace:
[201699.206982]  [<ffffffffc0907492>] nilfs_valid_sb.part.5+0x52/0x60 [nilfs2]
[201699.207773]  [<ffffffffc09075e2>] nilfs_load_super_block+0x142/0x300 [nilfs2]
[201699.208564]  [<ffffffff812479fd>] ? set_blocksize+0x9d/0xd0
[201699.209355]  [<ffffffffc0908020>] init_nilfs+0x60/0x390 [nilfs2]
[201699.210160]  [<ffffffffc08fc962>] nilfs_mount+0x302/0x520 [nilfs2]
[201699.210930]  [<ffffffff811b16a5>] ? pcpu_alloc+0x385/0x670
[201699.211685]  [<ffffffff81210c58>] mount_fs+0x38/0x160
[201699.212413]  [<ffffffff811b19c5>] ? __alloc_percpu+0x15/0x20
[201699.213151]  [<ffffffff8122cbe7>] vfs_kern_mount+0x67/0x110
[201699.213898]  [<ffffffff8122f3b9>] do_mount+0x269/0xe00
[201699.214671]  [<ffffffff8122d5a4>] ? mntput+0x24/0x40
[201699.215432]  [<ffffffff811ef064>] ? __kmalloc_track_caller+0x1b4/0x250
[201699.216207]  [<ffffffff8120eaf0>] ? __fput+0x190/0x220
[201699.216987]  [<ffffffff811ac0e2>] ? memdup_user+0x42/0x70
[201699.217777]  [<ffffffff8123027f>] SyS_mount+0x9f/0x100
[201699.218595]  [<ffffffff81825bf2>] entry_SYSCALL_64_fastpath+0x16/0x71
[201699.219405] Code: c0 00 00 00 49 89 d2 48 c1 ea 03 4c 8d 4e fc 41 83 e2 07 48 85 d2 74 7f 48 c1 e2 03 4c 8d 44 16 fc 4c 89 ce 8b 46 04 48 83 c6 08 <8b> 0e 31 f8 41 89 c3 0f b6 f8 0f b6 dc 41 c1 eb 18 8b 3c bd c0 
[201699.221341] RIP  [<ffffffff814083c6>] crc32_le+0x36/0x100
[201699.222300]  RSP <ffff8801216dbc50>
[201699.223246] CR2: ffff88021e600000
[201699.224200] ---[ end trace 1f71bf04a65dd7e5 ]---

A message that is not in the archive (sent off-list or not yet received): [email protected]

Torsten Hilbrich

author

2016-06-20 04:58 UTC #7 lore raw

PATCH v2 1/1 fs/nilfs2: Fix potential underflow in call to crc32_le

The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.

Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4.

This fixes a kernel bug when accidentially mounting an image file which had the nilfs2 magic value 0x3434 at the right offset 0x406 by chance. The bytes 0x01 0x00 were stored at 0x408 and were interpreted as a s_bytes value of 1. This caused an underflow when substracting sumoff + 4 (20) in the call to crc32_le.

[201699.185465] BUG: unable to handle kernel paging request at ffff88021e600000
[201699.186111] IP: [<ffffffff814083c6>] crc32_le+0x36/0x100
...
[201699.206202] Call Trace:
[201699.206982]  [<ffffffffc0907492>] nilfs_valid_sb.part.5+0x52/0x60 [nilfs2]
[201699.207773]  [<ffffffffc09075e2>] nilfs_load_super_block+0x142/0x300 [nilfs2]
[201699.208564]  [<ffffffff812479fd>] ? set_blocksize+0x9d/0xd0
[201699.209355]  [<ffffffffc0908020>] init_nilfs+0x60/0x390 [nilfs2]
[201699.210160]  [<ffffffffc08fc962>] nilfs_mount+0x302/0x520 [nilfs2]
[201699.210930]  [<ffffffff811b16a5>] ? pcpu_alloc+0x385/0x670
[201699.211685]  [<ffffffff81210c58>] mount_fs+0x38/0x160
[201699.212413]  [<ffffffff811b19c5>] ? __alloc_percpu+0x15/0x20
[201699.213151]  [<ffffffff8122cbe7>] vfs_kern_mount+0x67/0x110
[201699.213898]  [<ffffffff8122f3b9>] do_mount+0x269/0xe00
[201699.214671]  [<ffffffff8122d5a4>] ? mntput+0x24/0x40
[201699.215432]  [<ffffffff811ef064>] ? __kmalloc_track_caller+0x1b4/0x250
[201699.216207]  [<ffffffff8120eaf0>] ? __fput+0x190/0x220
[201699.216987]  [<ffffffff811ac0e2>] ? memdup_user+0x42/0x70
[201699.217777]  [<ffffffff8123027f>] SyS_mount+0x9f/0x100
[201699.218595]  [<ffffffff81825bf2>] entry_SYSCALL_64_fastpath+0x16/0x71

Signed-off-by Torsten Hilbrich <[email protected]>

Tested-by Torsten Hilbrich <[email protected]>

---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/nilfs2/the_nilfs.c +4 -3

index 69bd801..2de63db 100644
@@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	const size_t crc_start = sumoff + 4;
 	u32 crc;
 
 	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
 		return 0;
 	bytes = le16_to_cpu(sbp->s_bytes);
-	if (bytes > BLOCK_SIZE)
+	if (bytes < crc_start || bytes > BLOCK_SIZE)
 		return 0;
 	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
 		       sumoff);
 	crc = crc32_le(crc, sum, 4);
-	crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
-		       bytes - sumoff - 4);
+	crc = crc32_le(crc, (unsigned char *)sbp + crc_start,
+		       bytes - crc_start);
 	return crc == le32_to_cpu(sbp->s_sum);
 }

Signature · 7 lines

2.1.4


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ryusuke Konishi

2016-06-20 06:53 UTC #8 lore raw

In reply to a message not in the archive

Hi Torsten,

Thank you for your cooperation.

I saw the log and the patch, then
I have one more thing to ask you to change.

Could you drop the modifications related to the new constant "crc_start" or "crc_offset" ?

The position and the importance of your patch became clear since you inserted by the oops message and the commit log.

I now want the fix to be delivered also to stable-trees in order
to reflect it to old kernels. For it, the patch is recommended
to be simple and include only necessary change.

Thus, it's preferable to only include the change:

- if (bytes > BLOCK_SIZE)
+ if (bytes < sumoff + 4 || bytes > BLOCK_SIZE)

Regards,
Ryusuke Konishi

2016-06-19 21:41 GMT-07:00 Torsten Hilbrich <[email protected]>:

Quoted 96 lines · Ryusuke Konishi

Quoted 26 lines
Hi,
I have a few comments. Please see the inline comments below.
Quoted 10 lines · Torsten Hilbrich
The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.
Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4 and smaller than the remaining space in the block where the superblock is stored (BLOCK_SIZE - sumoff - 4).
Quoted 4 lines
This fixes a problem where the accidential mounting of an encrypted volume caused a kernel panic. It had the correct magic value 0x3434 at offset 0x406 by chance and the following 2 bytes were 0x01, 0x00 (interpreted as s_bytes value with value 1).
Could you improve the description on the problem a bit ?
The true issue looks a "memory access overrun" on the superblock buffer that can occur when "s_bytes - sumoff - 4" underflows and crc32_le() receives an uninteded large byte count.
Quoted 6 lines
The reason why a "kernel panic" occurs, is unclear in the comment.
Even if a non-nilfs volume is mistakenly mounted as a nilfs volume, it usually will return an error. Kernel panic should never occur just by an accidental mount. If it happens except the overrun issue, it is another bug.
You are right, I only got the kernel panic when first testing on a grsecurity kernel. I repeated the test and there just a BUG on the underflow/overrun is reported. I have attached the kernel logs of this test run to this mail (nilfs.bug).
Quoted 8 lines
Quoted 8 lines
Signed-off-by Torsten Hilbrich <[email protected]>
Tested-by Torsten Hilbrich <[email protected]>
---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Quoted 13 lines
fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 441
Quoted 9 lines
fs/nilfs2/the_nilfs.c +1 -0
index 69bd801..21f6b23 100644
@@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
     static unsigned char sum[4];
     const int sumoff = offsetof(struct nilfs_super_block, s_sum);
     size_t bytes;
+    size_t crc_offset = sumoff + 4;
The name "crc_offset" is confusing. This just means the offset of the latter part of the region that the crc function will verify. In addition, it should be defined with a "const" qualifier.
I will rename it to crc_start.
Quoted 8 lines
Quoted 7 lines
fs/nilfs2/the_nilfs.c +0 -1
     u32 crc;
 
     if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
             return 0;
     bytes = le16_to_cpu(sbp->s_bytes);
-    if (bytes > BLOCK_SIZE)
Quoted 4 lines
fs/nilfs2/the_nilfs.c static int nilfs_valid_sb(struct nilfs_super_block *sbp) · line 447
Quoted 1 line
fs/nilfs2/the_nilfs.c +1 -0
+    if (bytes < crc_offset || bytes > BLOCK_SIZE - crc_offset)
The latter part of this test should be "bytes > BLOCK_SIZE". Why do you subtract crc_offset here ?
I made here some wrong assumption about the memory layout and the meaning of bytes. bytes is the end of the area which is to be crc32-checked so comparing against BLOCK_SIZE (which should be the maximum size of the super block) is okay.
Quoted 15 lines
Regards,
Ryusuke Konishi
Quoted 11 lines
fs/nilfs2/the_nilfs.c +2 -2
             return 0;
     crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
                    sumoff);
     crc = crc32_le(crc, sum, 4);
-    crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
-                   bytes - sumoff - 4);
+    crc = crc32_le(crc, (unsigned char *)sbp + crc_offset,
+                   bytes - crc_offset);
     return crc == le32_to_cpu(sbp->s_sum);
 }
 

Quoted 9 lines

Quoted 3 lines
Quoted 3 lines
--
2.1.4
--
I will sent an updated patch.
Torsten

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAKFNMo=6A9Ns3EDe8PFL+LMX769WdGT39yo-p+Oi7gzPLG3-9w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Torsten Hilbrich

author

2016-06-20 07:01 UTC #10 lore raw

PATCH v3 1/1 fs/nilfs2: Fix potential underflow in call to crc32_le

The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.

Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4.

[201699.185465] BUG: unable to handle kernel paging request at
ffff88021e600000
[201699.186111] IP: [<ffffffff814083c6>] crc32_le+0x36/0x100
...
[201699.206202] Call Trace:
[201699.206982]  [<ffffffffc0907492>] nilfs_valid_sb.part.5+0x52/0x60
[nilfs2]
[201699.207773]  [<ffffffffc09075e2>] nilfs_load_super_block+0x142/0x300
[nilfs2]
[201699.208564]  [<ffffffff812479fd>] ? set_blocksize+0x9d/0xd0
[201699.209355]  [<ffffffffc0908020>] init_nilfs+0x60/0x390 [nilfs2]
[201699.210160]  [<ffffffffc08fc962>] nilfs_mount+0x302/0x520 [nilfs2]
[201699.210930]  [<ffffffff811b16a5>] ? pcpu_alloc+0x385/0x670
[201699.211685]  [<ffffffff81210c58>] mount_fs+0x38/0x160
[201699.212413]  [<ffffffff811b19c5>] ? __alloc_percpu+0x15/0x20
[201699.213151]  [<ffffffff8122cbe7>] vfs_kern_mount+0x67/0x110
[201699.213898]  [<ffffffff8122f3b9>] do_mount+0x269/0xe00
[201699.214671]  [<ffffffff8122d5a4>] ? mntput+0x24/0x40
[201699.215432]  [<ffffffff811ef064>] ? __kmalloc_track_caller+0x1b4/0x250
[201699.216207]  [<ffffffff8120eaf0>] ? __fput+0x190/0x220
[201699.216987]  [<ffffffff811ac0e2>] ? memdup_user+0x42/0x70
[201699.217777]  [<ffffffff8123027f>] SyS_mount+0x9f/0x100
[201699.218595]  [<ffffffff81825bf2>] entry_SYSCALL_64_fastpath+0x16/0x71

Signed-off-by Torsten Hilbrich <[email protected]>

Tested-by Torsten Hilbrich <[email protected]>

---
 fs/nilfs2/the_nilfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/nilfs2/the_nilfs.c +1 -1

index 69bd801..37e49cb 100644
@@ -443,7 +443,7 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
 	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
 		return 0;
 	bytes = le16_to_cpu(sbp->s_bytes);
-	if (bytes > BLOCK_SIZE)
+	if (bytes < sumoff + 4 || bytes > BLOCK_SIZE)
 		return 0;
 	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
 		       sumoff);

Signature · 7 lines

2.1.4


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ryusuke Konishi

2016-06-20 12:41 UTC #12 lore raw

Thank you!

Will apply this.

Regards,
Ryusuke Konishi

2016-06-20 0:01 GMT-07:00 Torsten Hilbrich <[email protected]>:

Quoted 60 lines

The value bytes comes from the filesystem which is about to be mounted. We cannot trust that the value is always in the range we expect it to be.

Check its value before using it to calculate the length for the crc32_le call. It value must be larger (or equal) sumoff + 4.

[201699.185465] BUG: unable to handle kernel paging request at
ffff88021e600000
[201699.186111] IP: [<ffffffff814083c6>] crc32_le+0x36/0x100
...
[201699.206202] Call Trace:
[201699.206982]  [<ffffffffc0907492>] nilfs_valid_sb.part.5+0x52/0x60
[nilfs2]
[201699.207773]  [<ffffffffc09075e2>] nilfs_load_super_block+0x142/0x300
[nilfs2]
[201699.208564]  [<ffffffff812479fd>] ? set_blocksize+0x9d/0xd0
[201699.209355]  [<ffffffffc0908020>] init_nilfs+0x60/0x390 [nilfs2]
[201699.210160]  [<ffffffffc08fc962>] nilfs_mount+0x302/0x520 [nilfs2]
[201699.210930]  [<ffffffff811b16a5>] ? pcpu_alloc+0x385/0x670
[201699.211685]  [<ffffffff81210c58>] mount_fs+0x38/0x160
[201699.212413]  [<ffffffff811b19c5>] ? __alloc_percpu+0x15/0x20
[201699.213151]  [<ffffffff8122cbe7>] vfs_kern_mount+0x67/0x110
[201699.213898]  [<ffffffff8122f3b9>] do_mount+0x269/0xe00
[201699.214671]  [<ffffffff8122d5a4>] ? mntput+0x24/0x40
[201699.215432]  [<ffffffff811ef064>] ? __kmalloc_track_caller+0x1b4/0x250
[201699.216207]  [<ffffffff8120eaf0>] ? __fput+0x190/0x220
[201699.216987]  [<ffffffff811ac0e2>] ? memdup_user+0x42/0x70
[201699.217777]  [<ffffffff8123027f>] SyS_mount+0x9f/0x100
[201699.218595]  [<ffffffff81825bf2>] entry_SYSCALL_64_fastpath+0x16/0x71

Signed-off-by Torsten Hilbrich <[email protected]>

Tested-by Torsten Hilbrich <[email protected]>

---
 fs/nilfs2/the_nilfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/nilfs2/the_nilfs.c +1 -2

index 69bd801..37e49cb 100644
@@ -443,7 +443,7 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
        if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
                return 0;
        bytes = le16_to_cpu(sbp->s_bytes);
-       if (bytes > BLOCK_SIZE)
+       if (bytes < sumoff + 4 || bytes > BLOCK_SIZE)
                return 0;
        crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
                       sumoff);
--

2.1.4

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html