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@xxxxxxxxxxx> >> Tested-by: Torsten Hilbrich <torsten.hilbrich@xxxxxxxxxxx> >> --- >> 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
[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 ]---
