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

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

[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 <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 69bd801..21f6b23 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -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);
 }
 
-- 
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs/nilfs2: Fix potential underflow in call to crc32_le

[Viacheslav Dubeyko]

On Fri, 2016-06-17 at 10:06 +0200, Torsten Hilbrich wrote:
> 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 <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
> ---
>  fs/nilfs2/the_nilfs.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 69bd801..21f6b23 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -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.

>  	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);
>  }
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs/nilfs2: Fix potential underflow in call to crc32_le

[Ryusuke Konishi]

Hi,

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

On Fri, 17 Jun 2016 10:06:45 +0200, Torsten Hilbrich wrote:
> 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).

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.

> 
> Signed-off-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
> ---
>  fs/nilfs2/the_nilfs.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 69bd801..21f6b23 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -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.

>  	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

>  		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);
>  }
>  
> -- 
> 2.1.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs/nilfs2: Fix potential underflow in call to crc32_le

[Torsten Hilbrich]

[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]

On 17.06.2016 20:25, Ryusuke Konishi wrote:
> Hi,
> 
> I have a few comments.  Please see the inline comments below.
> 
> On Fri, 17 Jun 2016 10:06:45 +0200, Torsten Hilbrich wrote:
>> 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).
> 
> 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.

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).

> 
>>
>> Signed-off-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
>> Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  fs/nilfs2/the_nilfs.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
>> index 69bd801..21f6b23 100644
>> --- a/fs/nilfs2/the_nilfs.c
>> +++ b/fs/nilfs2/the_nilfs.c
>> @@ -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.

> 
>>  	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 ?

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.

> 
> Regards,
> Ryusuke Konishi
> 
>>  		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);
>>  }
>>  
>> -- 
>> 2.1.4
>> --

I will sent an updated patch.

	Torsten



[-- Attachment #2: nilfs.bug --]
[-- Type: text/plain, Size: 4236 bytes --]

[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 ]---

[PATCHv2] fs/nilfs2: Fix potential underflow in call to crc32_le

[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.

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 <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
---
 fs/nilfs2/the_nilfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 69bd801..2de63db 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -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);
 }
 
-- 
2.1.4


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs/nilfs2: Fix potential underflow in call to crc32_le

[Ryusuke Konishi]

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 <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>:
> On 17.06.2016 20:25, Ryusuke Konishi wrote:
>> Hi,
>>
>> I have a few comments.  Please see the inline comments below.
>>
>> On Fri, 17 Jun 2016 10:06:45 +0200, Torsten Hilbrich wrote:
>>> 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).
>>
>> 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.
>
> 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).
>
>>
>>>
>>> Signed-off-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
>>> Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
>>> ---
>>>  fs/nilfs2/the_nilfs.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
>>> index 69bd801..21f6b23 100644
>>> --- a/fs/nilfs2/the_nilfs.c
>>> +++ b/fs/nilfs2/the_nilfs.c
>>> @@ -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.
>
>>
>>>      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 ?
>
> 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.
>
>>
>> Regards,
>> Ryusuke Konishi
>>
>>>              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);
>>>  }
>>>
>>> --
>>> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3] fs/nilfs2: Fix potential underflow in call to crc32_le

[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.

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 <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
---
 fs/nilfs2/the_nilfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 69bd801..37e49cb 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -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


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] fs/nilfs2: Fix potential underflow in call to crc32_le

[Ryusuke Konishi]

Thank you!

Will apply this.

Regards,
Ryusuke Konishi

2016-06-20 0:01 GMT-07:00 Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>:
> 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 <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
> ---
>  fs/nilfs2/the_nilfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 69bd801..37e49cb 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -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
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html