ext4 corruption causing a crash

ext4 corruption causing a crash

[Nikolay Borisov]

Hello, 

Recently I saw the following report : http://www.securityfocus.com/archive/1/539661

I've been testing that image and even on latest 4.9-rc4 I'm able to reproduce a crash. 
This seems rather indetermenistic in that it causes double/triple faults which causes 
my qemu guest to completely hang or reboot. However, on 4.9-rc4 with errors=panic 
mount options the crash looks like : 

[   47.172026] BUG: unable to handle kernel NULL pointer dereference at 000000000000090f
[   47.172215] IP: [<ffffffff81090e3f>] update_curr+0x2f/0x210
[   47.172342] PGD 0 [   47.172389] 
[   47.172438] Oops: 0000 [#1] SMP
[   47.172521] Modules linked in:
[   47.172627] CPU: 0 PID: 1108 Comm: mount Tainted: G        W       4.9.0-rc4-clouder1 #30
[   47.172777] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 04/01/2014
[   47.173008] task: ffff8800068ec100 task.stack: ffff880006648000
[   47.173008] RIP: 0010:[<ffffffff81090e3f>]  [<ffffffff81090e3f>] update_curr+0x2f/0x210
[   47.173008] RSP: 0018:ffff880007c03d68  EFLAGS: 00010082
[   47.173008] RAX: ffffffffffffffff RBX: ffff880006630000 RCX: 0000000afbabb8be
[   47.173008] RDX: 0000000000000000 RSI: ffff8800068ec100 RDI: ffff880006630000
[   47.173008] RBP: ffff880007c03da8 R08: 0000000000000022 R09: 0000000000000026
[   47.173008] R10: 00000000005acec3 R11: 0000000000000000 R12: 0000000000016300
[   47.173008] R13: ffffffffffffffff R14: ffff880006630000 R15: 0000000000000000
[   47.173008] FS:  00007f08d69e97e0(0000) GS:ffff880007c00000(0000) knlGS:0000000000000000
[   47.173008] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   47.173008] CR2: 000000000000090f CR3: 000000000665e000 CR4: 00000000000006f0
[   47.173008] Stack:
[   47.173008]  fffffffffffffed4 ffffffff820368e0 fffffffffffffed4 ffff8800068ec180
[   47.173008]  0000000000016300 0000000000000000 ffff880006630000 0000000000000000
[   47.173008]  ffff880007c03e18 ffffffff81097c84 ffff880007c03df8 ffff880007c16dc0
[   47.173008] Call Trace:
[   47.173008]  <IRQ> [   47.173008]  [<ffffffff81097c84>] task_tick_fair+0x2a4/0x3f0
[   47.173008]  [<ffffffff81084fa8>] scheduler_tick+0x68/0xe0
[   47.173008]  [<ffffffff810c1c81>] update_process_times+0x51/0x70
[   47.173008]  [<ffffffff810d2bf8>] tick_sched_timer+0x58/0x180
[   47.173008]  [<ffffffff810c3678>] ? __remove_hrtimer+0x58/0x90
[   47.173008]  [<ffffffff810c3f07>] __hrtimer_run_queues+0xd7/0x290
[   47.173008]  [<ffffffff810d2ba0>] ? tick_setup_sched_timer+0x100/0x100
[   47.173008]  [<ffffffff8103b14d>] ? lapic_next_event+0x1d/0x30
[   47.173008]  [<ffffffff810ca54b>] ? ktime_get_update_offsets_now+0x5b/0x120
[   47.173008]  [<ffffffff8106100f>] ? __local_bh_enable+0x3f/0x70
[   47.173008]  [<ffffffff810c4252>] hrtimer_interrupt+0xa2/0x1e0
[   47.173008]  [<ffffffff8103b8d9>] local_apic_timer_interrupt+0x39/0x60
[   47.173008]  [<ffffffff816ac3a1>] smp_apic_timer_interrupt+0x41/0x55
[   47.173008]  [<ffffffff816ab699>] apic_timer_interrupt+0x89/0x90
[   47.173008]  <EOI> [   47.173008]  [<ffffffff8127d834>] ? ext4_calculate_overhead+0x264/0x470
[   47.173008]  [<ffffffff8127d81e>] ? ext4_calculate_overhead+0x24e/0x470
[   47.173008]  [<ffffffff8128b979>] ext4_fill_super+0x2019/0x3270
[   47.173008]  [<ffffffff81344483>] ? pointer+0x2a3/0x400
[   47.173008]  [<ffffffff811d0477>] mount_bdev+0x187/0x1d0
[   47.173008]  [<ffffffff81170a65>] ? __alloc_percpu+0x15/0x20
[   47.173008]  [<ffffffff81289960>] ? ext4_alloc_flex_bg_array+0x110/0x110
[   47.173008]  [<ffffffff8127aec5>] ext4_mount+0x15/0x20
[   47.173008]  [<ffffffff811cf3d3>] mount_fs+0x43/0x170
[   47.173008]  [<ffffffff811ef6d6>] vfs_kern_mount+0x76/0x130
[   47.173008]  [<ffffffff811f05f6>] do_mount+0x226/0xcb0
[   47.173008]  [<ffffffff8116a707>] ? memdup_user+0x57/0x90
[   47.173008]  [<ffffffff811f10fa>] SyS_mount+0x7a/0xc0
[   47.173008]  [<ffffffff816a9dea>] entry_SYSCALL_64_fastpath+0x18/0xa8

Without it I usually get a DF in the PF handler (ouch). However, Sebastian 
reported that on his 4.9-rc4 it doesn't crash: 
http://paste.debian.net/hidden/6fae7231/

PeterZ, ffffffff81090e3f is 
kernel/sched/fair.c:2727 - val >>= local_n / LOAD_AVG_PERIOD; in decay_load
kernel/sched/fair.c:2850
kernel/sched/fair.c:3053

the 2727 line code seems very suspicious in that it doesn't work with 
any global memory hmmm...

On older 4.4.x based kernel the symptoms range from DF/TF to memory 
corruption in the scheduler, to a crash on the last return statement in 
count_overhead. It's still puzzling how come ext4 can cause such a far reaching
memory corruption that scheduler data structures are affected. 

Nikolay

Re: ext4 corruption causing a crash

[Nikolay Borisov]

On 11/16/2016 09:57 AM, Nikolay Borisov wrote:
> Hello, 
> 
> Recently I saw the following report : http://www.securityfocus.com/archive/1/539661
> 
> I've been testing that image and even on latest 4.9-rc4 I'm able to reproduce a crash. 
> This seems rather indetermenistic in that it causes double/triple faults which causes 
> my qemu guest to completely hang or reboot. However, on 4.9-rc4 with errors=panic 
> mount options the crash looks like : 
> 
> [   47.172026] BUG: unable to handle kernel NULL pointer dereference at 000000000000090f
> [   47.172215] IP: [<ffffffff81090e3f>] update_curr+0x2f/0x210
> [   47.172342] PGD 0 [   47.172389] 
> [   47.172438] Oops: 0000 [#1] SMP
> [   47.172521] Modules linked in:
> [   47.172627] CPU: 0 PID: 1108 Comm: mount Tainted: G        W       4.9.0-rc4-clouder1 #30
> [   47.172777] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 04/01/2014
> [   47.173008] task: ffff8800068ec100 task.stack: ffff880006648000
> [   47.173008] RIP: 0010:[<ffffffff81090e3f>]  [<ffffffff81090e3f>] update_curr+0x2f/0x210
> [   47.173008] RSP: 0018:ffff880007c03d68  EFLAGS: 00010082
> [   47.173008] RAX: ffffffffffffffff RBX: ffff880006630000 RCX: 0000000afbabb8be
> [   47.173008] RDX: 0000000000000000 RSI: ffff8800068ec100 RDI: ffff880006630000
> [   47.173008] RBP: ffff880007c03da8 R08: 0000000000000022 R09: 0000000000000026
> [   47.173008] R10: 00000000005acec3 R11: 0000000000000000 R12: 0000000000016300
> [   47.173008] R13: ffffffffffffffff R14: ffff880006630000 R15: 0000000000000000
> [   47.173008] FS:  00007f08d69e97e0(0000) GS:ffff880007c00000(0000) knlGS:0000000000000000
> [   47.173008] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   47.173008] CR2: 000000000000090f CR3: 000000000665e000 CR4: 00000000000006f0
> [   47.173008] Stack:
> [   47.173008]  fffffffffffffed4 ffffffff820368e0 fffffffffffffed4 ffff8800068ec180
> [   47.173008]  0000000000016300 0000000000000000 ffff880006630000 0000000000000000
> [   47.173008]  ffff880007c03e18 ffffffff81097c84 ffff880007c03df8 ffff880007c16dc0
> [   47.173008] Call Trace:
> [   47.173008]  <IRQ> [   47.173008]  [<ffffffff81097c84>] task_tick_fair+0x2a4/0x3f0
> [   47.173008]  [<ffffffff81084fa8>] scheduler_tick+0x68/0xe0
> [   47.173008]  [<ffffffff810c1c81>] update_process_times+0x51/0x70
> [   47.173008]  [<ffffffff810d2bf8>] tick_sched_timer+0x58/0x180
> [   47.173008]  [<ffffffff810c3678>] ? __remove_hrtimer+0x58/0x90
> [   47.173008]  [<ffffffff810c3f07>] __hrtimer_run_queues+0xd7/0x290
> [   47.173008]  [<ffffffff810d2ba0>] ? tick_setup_sched_timer+0x100/0x100
> [   47.173008]  [<ffffffff8103b14d>] ? lapic_next_event+0x1d/0x30
> [   47.173008]  [<ffffffff810ca54b>] ? ktime_get_update_offsets_now+0x5b/0x120
> [   47.173008]  [<ffffffff8106100f>] ? __local_bh_enable+0x3f/0x70
> [   47.173008]  [<ffffffff810c4252>] hrtimer_interrupt+0xa2/0x1e0
> [   47.173008]  [<ffffffff8103b8d9>] local_apic_timer_interrupt+0x39/0x60
> [   47.173008]  [<ffffffff816ac3a1>] smp_apic_timer_interrupt+0x41/0x55
> [   47.173008]  [<ffffffff816ab699>] apic_timer_interrupt+0x89/0x90
> [   47.173008]  <EOI> [   47.173008]  [<ffffffff8127d834>] ? ext4_calculate_overhead+0x264/0x470
> [   47.173008]  [<ffffffff8127d81e>] ? ext4_calculate_overhead+0x24e/0x470
> [   47.173008]  [<ffffffff8128b979>] ext4_fill_super+0x2019/0x3270
> [   47.173008]  [<ffffffff81344483>] ? pointer+0x2a3/0x400
> [   47.173008]  [<ffffffff811d0477>] mount_bdev+0x187/0x1d0
> [   47.173008]  [<ffffffff81170a65>] ? __alloc_percpu+0x15/0x20
> [   47.173008]  [<ffffffff81289960>] ? ext4_alloc_flex_bg_array+0x110/0x110
> [   47.173008]  [<ffffffff8127aec5>] ext4_mount+0x15/0x20
> [   47.173008]  [<ffffffff811cf3d3>] mount_fs+0x43/0x170
> [   47.173008]  [<ffffffff811ef6d6>] vfs_kern_mount+0x76/0x130
> [   47.173008]  [<ffffffff811f05f6>] do_mount+0x226/0xcb0
> [   47.173008]  [<ffffffff8116a707>] ? memdup_user+0x57/0x90
> [   47.173008]  [<ffffffff811f10fa>] SyS_mount+0x7a/0xc0
> [   47.173008]  [<ffffffff816a9dea>] entry_SYSCALL_64_fastpath+0x18/0xa8
> 
> Without it I usually get a DF in the PF handler (ouch). However, Sebastian 
> reported that on his 4.9-rc4 it doesn't crash: 
> http://paste.debian.net/hidden/6fae7231/
> 
> PeterZ, ffffffff81090e3f is 
> kernel/sched/fair.c:2727 - val >>= local_n / LOAD_AVG_PERIOD; in decay_load
> kernel/sched/fair.c:2850
> kernel/sched/fair.c:3053

This is the result with KASAN enabled, without the IP is a bit different
it shows:

/home/projects/linux-stable/kernel/sched/fair.c:281
/home/projects/linux-stable/kernel/sched/fair.c:3070
/home/projects/linux-stable/kernel/sched/fair.c:3678
/home/projects/linux-stable/kernel/sched/fair.c:8599

line 281 being cfs_rq_of, meaning sched_entity pointer is corrupted.

> 
> the 2727 line code seems very suspicious in that it doesn't work with 
> any global memory hmmm...
> 
> On older 4.4.x based kernel the symptoms range from DF/TF to memory 
> corruption in the scheduler, to a crash on the last return statement in 
> count_overhead. It's still puzzling how come ext4 can cause such a far reaching
> memory corruption that scheduler data structures are affected. 
> 
> Nikolay
> 
> 
> 
> 
> 

Re: ext4 corruption causing a crash

[Nikolay Borisov]

On 11/16/2016 10:12 AM, Nikolay Borisov wrote:
> 
> 
> On 11/16/2016 09:57 AM, Nikolay Borisov wrote:
>> Hello, 
>>
>> Recently I saw the following report : http://www.securityfocus.com/archive/1/539661
>>
>> I've been testing that image and even on latest 4.9-rc4 I'm able to reproduce a crash. 
>> This seems rather indetermenistic in that it causes double/triple faults which causes 
>> my qemu guest to completely hang or reboot. However, on 4.9-rc4 with errors=panic 
>> mount options the crash looks like : 
>>
>> [   47.172026] BUG: unable to handle kernel NULL pointer dereference at 000000000000090f
>> [   47.172215] IP: [<ffffffff81090e3f>] update_curr+0x2f/0x210
>> [   47.172342] PGD 0 [   47.172389] 
>> [   47.172438] Oops: 0000 [#1] SMP
>> [   47.172521] Modules linked in:
>> [   47.172627] CPU: 0 PID: 1108 Comm: mount Tainted: G        W       4.9.0-rc4-clouder1 #30
>> [   47.172777] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 04/01/2014
>> [   47.173008] task: ffff8800068ec100 task.stack: ffff880006648000
>> [   47.173008] RIP: 0010:[<ffffffff81090e3f>]  [<ffffffff81090e3f>] update_curr+0x2f/0x210
>> [   47.173008] RSP: 0018:ffff880007c03d68  EFLAGS: 00010082
>> [   47.173008] RAX: ffffffffffffffff RBX: ffff880006630000 RCX: 0000000afbabb8be
>> [   47.173008] RDX: 0000000000000000 RSI: ffff8800068ec100 RDI: ffff880006630000
>> [   47.173008] RBP: ffff880007c03da8 R08: 0000000000000022 R09: 0000000000000026
>> [   47.173008] R10: 00000000005acec3 R11: 0000000000000000 R12: 0000000000016300
>> [   47.173008] R13: ffffffffffffffff R14: ffff880006630000 R15: 0000000000000000
>> [   47.173008] FS:  00007f08d69e97e0(0000) GS:ffff880007c00000(0000) knlGS:0000000000000000
>> [   47.173008] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   47.173008] CR2: 000000000000090f CR3: 000000000665e000 CR4: 00000000000006f0
>> [   47.173008] Stack:
>> [   47.173008]  fffffffffffffed4 ffffffff820368e0 fffffffffffffed4 ffff8800068ec180
>> [   47.173008]  0000000000016300 0000000000000000 ffff880006630000 0000000000000000
>> [   47.173008]  ffff880007c03e18 ffffffff81097c84 ffff880007c03df8 ffff880007c16dc0
>> [   47.173008] Call Trace:
>> [   47.173008]  <IRQ> [   47.173008]  [<ffffffff81097c84>] task_tick_fair+0x2a4/0x3f0
>> [   47.173008]  [<ffffffff81084fa8>] scheduler_tick+0x68/0xe0
>> [   47.173008]  [<ffffffff810c1c81>] update_process_times+0x51/0x70
>> [   47.173008]  [<ffffffff810d2bf8>] tick_sched_timer+0x58/0x180
>> [   47.173008]  [<ffffffff810c3678>] ? __remove_hrtimer+0x58/0x90
>> [   47.173008]  [<ffffffff810c3f07>] __hrtimer_run_queues+0xd7/0x290
>> [   47.173008]  [<ffffffff810d2ba0>] ? tick_setup_sched_timer+0x100/0x100
>> [   47.173008]  [<ffffffff8103b14d>] ? lapic_next_event+0x1d/0x30
>> [   47.173008]  [<ffffffff810ca54b>] ? ktime_get_update_offsets_now+0x5b/0x120
>> [   47.173008]  [<ffffffff8106100f>] ? __local_bh_enable+0x3f/0x70
>> [   47.173008]  [<ffffffff810c4252>] hrtimer_interrupt+0xa2/0x1e0
>> [   47.173008]  [<ffffffff8103b8d9>] local_apic_timer_interrupt+0x39/0x60
>> [   47.173008]  [<ffffffff816ac3a1>] smp_apic_timer_interrupt+0x41/0x55
>> [   47.173008]  [<ffffffff816ab699>] apic_timer_interrupt+0x89/0x90
>> [   47.173008]  <EOI> [   47.173008]  [<ffffffff8127d834>] ? ext4_calculate_overhead+0x264/0x470
>> [   47.173008]  [<ffffffff8127d81e>] ? ext4_calculate_overhead+0x24e/0x470
>> [   47.173008]  [<ffffffff8128b979>] ext4_fill_super+0x2019/0x3270
>> [   47.173008]  [<ffffffff81344483>] ? pointer+0x2a3/0x400
>> [   47.173008]  [<ffffffff811d0477>] mount_bdev+0x187/0x1d0
>> [   47.173008]  [<ffffffff81170a65>] ? __alloc_percpu+0x15/0x20
>> [   47.173008]  [<ffffffff81289960>] ? ext4_alloc_flex_bg_array+0x110/0x110
>> [   47.173008]  [<ffffffff8127aec5>] ext4_mount+0x15/0x20
>> [   47.173008]  [<ffffffff811cf3d3>] mount_fs+0x43/0x170
>> [   47.173008]  [<ffffffff811ef6d6>] vfs_kern_mount+0x76/0x130
>> [   47.173008]  [<ffffffff811f05f6>] do_mount+0x226/0xcb0
>> [   47.173008]  [<ffffffff8116a707>] ? memdup_user+0x57/0x90
>> [   47.173008]  [<ffffffff811f10fa>] SyS_mount+0x7a/0xc0
>> [   47.173008]  [<ffffffff816a9dea>] entry_SYSCALL_64_fastpath+0x18/0xa8
>>


Ok so with the help of bpetkov we managed to get to the bottom it, 
essentially what happens is that we overflow the page allocated in 
ext4_calculate_overhead while working in the following loop in count_overhead: 

for (j = ext4_bg_num_gdb(sb, grp); j > 0; j--) {
	ext4_set_bit(EXT4_B2C(sbi, s++), buf);
	count++;
}

so ext4_bg_num_gdb returns a big number - 32768 and since sbi->s_cluster_bits 
is 0 the result of EXT4_B2C(sbi, s++) becomes just s, which incremented on 
every iteration and eventually becomes a large number. This causes to overflow 
the page used as a buf. This can be seen easily from the crash produced when 
debug_pagealloc is enabled: 

[   18.607773] BUG: unable to handle kernel paging request at ffff880005bcb000
[   18.607948] IP: [<ffffffff8127caad>] ext4_calculate_overhead+0x51d/0x540
[   18.608097] PGD 206f067 
[   18.608097] PUD 2070067 
[   18.608097] PMD 7dd3067 
[   18.608097] PTE 8000000005bcb060
[   18.608097] 
[   18.608097] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
[   18.608097] Modules linked in:
[   18.608097] CPU: 0 PID: 1164 Comm: mount Tainted: G        W       4.9.0-rc4-clouder1 #40
[   18.608097] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 04/01/2014
[   18.608097] task: ffff88000632d8c0 task.stack: ffffc900002e4000
[   18.608097] RIP: 0010:[<ffffffff8127caad>]  [<ffffffff8127caad>] ext4_calculate_overhead+0x51d/0x540
[   18.608097] RSP: 0018:ffffc900002e7b68  EFLAGS: 00010296
[   18.608097] RAX: 0000000000000020 RBX: 0000000000008001 RCX: ffffffff81c40e28
[   18.608097] RDX: 0000000000000001 RSI: 0000000000000282 RDI: ffffffff81e35060
[   18.608097] RBP: ffffc900002e7bf8 R08: 0000000000008000 R09: 000000000000048c
[   18.608097] R10: 0000000000000000 R11: 0000000000000002 R12: ffff880005bca000
[   18.608097] R13: ffff880005f95d28 R14: ffff880005f953d8 R15: 0000000000008004
[   18.608097] FS:  00007f90c676e7e0(0000) GS:ffff880007a00000(0000) knlGS:0000000000000000
[   18.608097] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   18.608097] CR2: ffff880005bcb000 CR3: 00000000054ee000 CR4: 00000000000006f0
[   18.608097] Stack:
[   18.608097]  ffffc90000000001 ffffffff00008000 0000000000001fff 0000000000000000
[   18.608097]  3231b20100000000 ffff880005bca001 00000000002e7be8 0000000000000000
[   18.608097]  ffff880005f95d28 00000001812a88fd 0000000000000000 0000000000000001
[   18.608097] Call Trace:
[   18.608097]  [<ffffffff8128aa19>] ext4_fill_super+0x2019/0x3270
[   18.608097]  [<ffffffff813434f3>] ? pointer+0x2a3/0x400
[   18.608097]  [<ffffffff811cf427>] mount_bdev+0x187/0x1d0
[   18.608097]  [<ffffffff811711b5>] ? __alloc_percpu+0x15/0x20
[   18.608097]  [<ffffffff81288a00>] ? ext4_alloc_flex_bg_array+0x110/0x110
[   18.608097]  [<ffffffff81279e85>] ext4_mount+0x15/0x20
[   18.608097]  [<ffffffff811ce383>] mount_fs+0x43/0x170
[   18.608097]  [<ffffffff811ee686>] vfs_kern_mount+0x76/0x130
[   18.608097]  [<ffffffff811ef5a6>] do_mount+0x226/0xcb0
[   18.608097]  [<ffffffff81083e53>] ? __might_sleep+0x53/0xa0
[   18.608097]  [<ffffffff8117c9d1>] ? __might_fault+0x31/0x40
[   18.608097]  [<ffffffff8116ae57>] ? memdup_user+0x57/0x90
[   18.608097]  [<ffffffff8116aee3>] ? strndup_user+0x53/0x70
[   18.608097]  [<ffffffff811f00aa>] SyS_mount+0x7a/0xc0
[   18.608097]  [<ffffffff816a23ea>] entry_SYSCALL_64_fastpath+0x18/0xa8


The crashing code: 

2b:*	49 0f ab 0c 24       	bts    %rcx,(%r12)		<-- trapping instruction

so RCX is the bit we want to set and r12 would be the page: 
R12: ffff880005940000 
RCX: 0000000000008000

However, the faulting address is actually the next page, following the 
one we allocated in ext4_calculate_overhead: 
CR2: ffff880005941000

Given this is a crafted image I guess there is inadequate validation 
of the s_cluster_bit as well as the values in ext4_bg_num_gdb, since this
function is used to get the number of iterations in calculate_overhead. 



Regards, 
Nikolay 

Re: ext4 corruption causing a crash

[Theodore Ts'o]

On Wed, Nov 16, 2016 at 03:03:35PM +0200, Nikolay Borisov wrote:
> 
> Ok so with the help of bpetkov we managed to get to the bottom it, 
> essentially what happens is that we overflow the page allocated in 
> ext4_calculate_overhead while working in the following loop in count_overhead: 
> 
> for (j = ext4_bg_num_gdb(sb, grp); j > 0; j--) {
> 	ext4_set_bit(EXT4_B2C(sbi, s++), buf);
> 	count++;
> }
> 
> so ext4_bg_num_gdb returns a big number - 32768 and since sbi->s_cluster_bits 
> is 0 the result of EXT4_B2C(sbi, s++) becomes just s, which incremented on 
> every iteration and eventually becomes a large number. This causes to overflow 
> the page used as a buf. This can be seen easily from the crash produced when 
> debug_pagealloc is enabled:

Thanks for digging into this!  It sounds like we need to add more
sanity checking at mount time, as well as some safety constraints to
make sure we don't overrun memory.

Can you send me dumpe2fs -h output from the drive that provoked this?

Thanks,

						- Ted

Re: ext4 corruption causing a crash

[Nikolay Borisov]

On 11/16/2016 08:17 PM, Theodore Ts'o wrote:
> On Wed, Nov 16, 2016 at 03:03:35PM +0200, Nikolay Borisov wrote:
>>
>> Ok so with the help of bpetkov we managed to get to the bottom it, 
>> essentially what happens is that we overflow the page allocated in 
>> ext4_calculate_overhead while working in the following loop in count_overhead: 
>>
>> for (j = ext4_bg_num_gdb(sb, grp); j > 0; j--) {
>> 	ext4_set_bit(EXT4_B2C(sbi, s++), buf);
>> 	count++;
>> }
>>
>> so ext4_bg_num_gdb returns a big number - 32768 and since sbi->s_cluster_bits 
>> is 0 the result of EXT4_B2C(sbi, s++) becomes just s, which incremented on 
>> every iteration and eventually becomes a large number. This causes to overflow 
>> the page used as a buf. This can be seen easily from the crash produced when 
>> debug_pagealloc is enabled:
> 
> Thanks for digging into this!  It sounds like we need to add more
> sanity checking at mount time, as well as some safety constraints to
> make sure we don't overrun memory.
> 
> Can you send me dumpe2fs -h output from the drive that provoked this?

Sorry, no. 

dumpe2fs -h -f OSS-2016-22-image 
dumpe2fs 1.42.12 (29-Aug-2014)
dumpe2fs: Filesystem revision too high while trying to open OSS-2016-22-image
Couldn't find valid filesystem superblock.

Here is what 'file' reports though: 
 
OSS-2016-22-image: Linux rev 65537.0 ext2 filesystem data (extents) (huge files)

Also you can find the offending image here: 
https://os-s.net/advisories/OSS-2016-23-image


> 
> Thanks,
> 
> 						- Ted
> 

[PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

If the block size or cluster size is insane, reject the mount.  This
is important for security reasons (although we shouldn't be just
depending on this check).

Ref: http://www.securityfocus.com/archive/1/539661
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1332506
Reported-by: Borislav Petkov <bp@alien8.de>
Reported-by: Nikolay Borisov <kernel@kyup.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/super.c | 17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 53d6d463ac4d..bdf1e5ee8642 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -235,6 +235,7 @@ struct ext4_io_submit {
 #define	EXT4_MAX_BLOCK_SIZE		65536
 #define EXT4_MIN_BLOCK_LOG_SIZE		10
 #define EXT4_MAX_BLOCK_LOG_SIZE		16
+#define EXT4_MAX_CLUSTER_LOG_SIZE	30
 #ifdef __KERNEL__
 # define EXT4_BLOCK_SIZE(s)		((s)->s_blocksize)
 #else
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 35ccbdc2d64e..12f50ef56fe1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3567,7 +3567,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
 	    blocksize > EXT4_MAX_BLOCK_SIZE) {
 		ext4_msg(sb, KERN_ERR,
-		       "Unsupported filesystem blocksize %d", blocksize);
+		       "Unsupported filesystem blocksize %d (%d)",
+			 blocksize, le32_to_cpu(es->s_log_block_size));
+		goto failed_mount;
+	}
+	if (le32_to_cpu(es->s_log_block_size) >
+	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
+		ext4_msg(sb, KERN_ERR,
+			 "Invalid log block size: %u",
+			 le32_to_cpu(es->s_log_block_size));
 		goto failed_mount;
 	}
 
@@ -3699,6 +3707,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 				 "block size (%d)", clustersize, blocksize);
 			goto failed_mount;
 		}
+		if (le32_to_cpu(es->s_log_cluster_size) >
+		    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
+			ext4_msg(sb, KERN_ERR,
+				 "Invalid log cluster size: %u",
+				 le32_to_cpu(es->s_log_cluster_size));
+			goto failed_mount;
+		}
 		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
 			le32_to_cpu(es->s_log_block_size);
 		sbi->s_clusters_per_group =
-- 
2.11.0.rc0.7.gbe5a750

[PATCH 2/3] ext4: fix in-superblock mount options processing

[Theodore Ts'o]

Fix a large number of problems with how we handle mount options in the
superblock.  For one, if the string in the superblock is long enough
that it is not null terminated, we could run off the end of the string
and try to interpret superblocks fields as characters.  It's unlikely
this will cause a security problem, but it could result in an invalid
parse.  Also, parse_options is destructive to the string, so in some
cases if there is a comma-separated string, it would be modified in
the superblock.  (Fortunately it only happens on file systems with a
1k block size.)

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/super.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 12f50ef56fe1..3009c03220d6 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3303,7 +3303,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	char *orig_data = kstrdup(data, GFP_KERNEL);
 	struct buffer_head *bh;
 	struct ext4_super_block *es = NULL;
-	struct ext4_sb_info *sbi;
+	struct ext4_sb_info *sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	ext4_fsblk_t block;
 	ext4_fsblk_t sb_block = get_sb_block(&data);
 	ext4_fsblk_t logical_sb_block;
@@ -3321,17 +3321,17 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	int err = 0;
 	unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
 	ext4_group_t first_not_zeroed;
+	char *s_mount_opts = kzalloc(sizeof(sbi->s_es->s_mount_opts)+1,
+				   GFP_KERNEL);
 
-	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
-	if (!sbi)
-		goto out_free_orig;
+	if ((data && !orig_data) || !s_mount_opts || !sbi)
+		goto out_free_base;
 
 	sbi->s_blockgroup_lock =
 		kzalloc(sizeof(struct blockgroup_lock), GFP_KERNEL);
-	if (!sbi->s_blockgroup_lock) {
-		kfree(sbi);
-		goto out_free_orig;
-	}
+	if (!sbi->s_blockgroup_lock)
+		goto out_free_base;
+
 	sb->s_fs_info = sbi;
 	sbi->s_sb = sb;
 	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
@@ -3477,11 +3477,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 */
 	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
 
-	if (!parse_options((char *) sbi->s_es->s_mount_opts, sb,
-			   &journal_devnum, &journal_ioprio, 0)) {
-		ext4_msg(sb, KERN_WARNING,
-			 "failed to parse options in superblock: %s",
-			 sbi->s_es->s_mount_opts);
+	if (sbi->s_es->s_mount_opts[0]) {
+		strncpy(s_mount_opts, sbi->s_es->s_mount_opts,
+			sizeof(sbi->s_es->s_mount_opts));
+		if (!parse_options(s_mount_opts, sb, &journal_devnum,
+				   &journal_ioprio, 0)) {
+			ext4_msg(sb, KERN_WARNING,
+				 "failed to parse options in superblock: %s",
+				 s_mount_opts);
+		}
 	}
 	sbi->s_def_mount_opt = sbi->s_mount_opt;
 	if (!parse_options((char *) data, sb, &journal_devnum,
@@ -4162,7 +4166,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 
 	if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount"))
 		ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
-			 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
+			 "Opts: %.*s%s%s", descr,
+			 sizeof(sbi->s_es->s_mount_opts),
+			 sbi->s_es->s_mount_opts,
 			 *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
 
 	if (es->s_error_count)
@@ -4241,9 +4247,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 out_fail:
 	sb->s_fs_info = NULL;
 	kfree(sbi->s_blockgroup_lock);
+out_free_base:
 	kfree(sbi);
-out_free_orig:
 	kfree(orig_data);
+	kfree(s_mount_opts);
 	return err ? err : ret;
 }
 
-- 
2.11.0.rc0.7.gbe5a750

[PATCH 3/3] ext4: use more strict checks for inodes_per_block on mount

[Theodore Ts'o]

Centralize the checks for inodes_per_block and be more strict to make
sure the inodes_per_block_group can't end up being zero.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/super.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3009c03220d6..6638a0415997 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3666,12 +3666,16 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 
 	sbi->s_blocks_per_group = le32_to_cpu(es->s_blocks_per_group);
 	sbi->s_inodes_per_group = le32_to_cpu(es->s_inodes_per_group);
-	if (EXT4_INODE_SIZE(sb) == 0 || EXT4_INODES_PER_GROUP(sb) == 0)
-		goto cantfind_ext4;
 
 	sbi->s_inodes_per_block = blocksize / EXT4_INODE_SIZE(sb);
 	if (sbi->s_inodes_per_block == 0)
 		goto cantfind_ext4;
+	if (sbi->s_inodes_per_group < sbi->s_inodes_per_block ||
+	    sbi->s_inodes_per_group > blocksize * 8) {
+		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
+			 sbi->s_blocks_per_group);
+		goto failed_mount;
+	}
 	sbi->s_itb_per_group = sbi->s_inodes_per_group /
 					sbi->s_inodes_per_block;
 	sbi->s_desc_per_block = blocksize / EXT4_DESC_SIZE(sb);
@@ -3754,13 +3758,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	sbi->s_cluster_ratio = clustersize / blocksize;
 
-	if (sbi->s_inodes_per_group > blocksize * 8) {
-		ext4_msg(sb, KERN_ERR,
-		       "#inodes per group too big: %lu",
-		       sbi->s_inodes_per_group);
-		goto failed_mount;
-	}
-
 	/* Do we have standard group size of clustersize * 8 blocks ? */
 	if (sbi->s_blocks_per_group == clustersize << 3)
 		set_opt2(sb, STD_GROUP_SIZE);
-- 
2.11.0.rc0.7.gbe5a750

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Nikolay Borisov]

On 11/18/2016 06:26 AM, Theodore Ts'o wrote:
> If the block size or cluster size is insane, reject the mount.  This
> is important for security reasons (although we shouldn't be just
> depending on this check).
> 
> Ref: http://www.securityfocus.com/archive/1/539661
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1332506
> Reported-by: Borislav Petkov <bp@alien8.de>
> Reported-by: Nikolay Borisov <kernel@kyup.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@vger.kernel.org

I just tested with the 3 patches applied on 4.9-rc4 and could still
reproduce the issue.

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Andreas Dilger]

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

On Nov 17, 2016, at 9:26 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> If the block size or cluster size is insane, reject the mount.  This
> is important for security reasons (although we shouldn't be just
> depending on this check).
> 
> Ref: http://www.securityfocus.com/archive/1/539661
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1332506
> Reported-by: Borislav Petkov <bp@alien8.de>
> Reported-by: Nikolay Borisov <kernel@kyup.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@vger.kernel.org
> ---
> fs/ext4/ext4.h  |  1 +
> fs/ext4/super.c | 17 ++++++++++++++++-
> 2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 35ccbdc2d64e..12f50ef56fe1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3567,7 +3567,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
> 	    blocksize > EXT4_MAX_BLOCK_SIZE) {
> 		ext4_msg(sb, KERN_ERR,
> -		       "Unsupported filesystem blocksize %d", blocksize);
> +		       "Unsupported filesystem blocksize %d (%d)",

Would be good to indicate what the second value is, like "(%d bits)".

Cheers, Andreas

> +			 blocksize, le32_to_cpu(es->s_log_block_size));
> +		goto failed_mount;
> +	}
> +	if (le32_to_cpu(es->s_log_block_size) >
> +	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "Invalid log block size: %u",
> +			 le32_to_cpu(es->s_log_block_size));
> 		goto failed_mount;
> 	}
> 
> @@ -3699,6 +3707,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 				 "block size (%d)", clustersize, blocksize);
> 			goto failed_mount;
> 		}
> +		if (le32_to_cpu(es->s_log_cluster_size) >
> +		    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "Invalid log cluster size: %u",
> +				 le32_to_cpu(es->s_log_cluster_size));
> +			goto failed_mount;
> +		}
> 		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
> 			le32_to_cpu(es->s_log_block_size);
> 		sbi->s_clusters_per_group =
> --
> 2.11.0.rc0.7.gbe5a750
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] ext4: fix in-superblock mount options processing

[Andreas Dilger]

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

On Nov 17, 2016, at 9:26 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> Fix a large number of problems with how we handle mount options in the
> superblock.  For one, if the string in the superblock is long enough
> that it is not null terminated, we could run off the end of the string
> and try to interpret superblocks fields as characters.  It's unlikely
> this will cause a security problem, but it could result in an invalid
> parse.  Also, parse_options is destructive to the string, so in some
> cases if there is a comma-separated string, it would be modified in
> the superblock.  (Fortunately it only happens on file systems with a
> 1k block size.)
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@vger.kernel.org
> ---
> fs/ext4/super.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 12f50ef56fe1..3009c03220d6 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3303,7 +3303,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	char *orig_data = kstrdup(data, GFP_KERNEL);
> 	struct buffer_head *bh;
> 	struct ext4_super_block *es = NULL;
> -	struct ext4_sb_info *sbi;
> +	struct ext4_sb_info *sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> 	ext4_fsblk_t block;
> 	ext4_fsblk_t sb_block = get_sb_block(&data);
> 	ext4_fsblk_t logical_sb_block;
> @@ -3321,17 +3321,17 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	int err = 0;
> 	unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> 	ext4_group_t first_not_zeroed;
> +	char *s_mount_opts = kzalloc(sizeof(sbi->s_es->s_mount_opts)+1,
> +				   GFP_KERNEL);

This doesn't check if "sbi" is non-NULL before dereferencing it.  While
there isn't really any likelihood for this allocation to fail, I don't
think there is any benefit to moving these allocations to the declaration.

> -	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> -	if (!sbi)
> -		goto out_free_orig;
> +	if ((data && !orig_data) || !s_mount_opts || !sbi)
> +		goto out_free_base;
> 
> 	sbi->s_blockgroup_lock =
> 		kzalloc(sizeof(struct blockgroup_lock), GFP_KERNEL);
> -	if (!sbi->s_blockgroup_lock) {
> -		kfree(sbi);
> -		goto out_free_orig;
> -	}
> +	if (!sbi->s_blockgroup_lock)
> +		goto out_free_base;
> +
> 	sb->s_fs_info = sbi;
> 	sbi->s_sb = sb;
> 	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
> @@ -3477,11 +3477,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	 */
> 	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
> 
> -	if (!parse_options((char *) sbi->s_es->s_mount_opts, sb,
> -			   &journal_devnum, &journal_ioprio, 0)) {
> -		ext4_msg(sb, KERN_WARNING,
> -			 "failed to parse options in superblock: %s",
> -			 sbi->s_es->s_mount_opts);
> +	if (sbi->s_es->s_mount_opts[0]) {
> +		strncpy(s_mount_opts, sbi->s_es->s_mount_opts,
> +			sizeof(sbi->s_es->s_mount_opts));

This could use kstrndup()?  It always allocates an extra byte for the NUL.

> +		if (!parse_options(s_mount_opts, sb, &journal_devnum,
> +				   &journal_ioprio, 0)) {
> +			ext4_msg(sb, KERN_WARNING,
> +				 "failed to parse options in superblock: %s",
> +				 s_mount_opts);
> +		}
> 	}
> 	sbi->s_def_mount_opt = sbi->s_mount_opt;
> 	if (!parse_options((char *) data, sb, &journal_devnum,
> @@ -4162,7 +4166,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 
> 	if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount"))
> 		ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> -			 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> +			 "Opts: %.*s%s%s", descr,
> +			 sizeof(sbi->s_es->s_mount_opts),
> +			 sbi->s_es->s_mount_opts,

This isn't really needed, since there is always a NUL terminator added
to the string?

> 			 *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> 
> 	if (es->s_error_count)
> @@ -4241,9 +4247,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> out_fail:
> 	sb->s_fs_info = NULL;
> 	kfree(sbi->s_blockgroup_lock);
> +out_free_base:
> 	kfree(sbi);
> -out_free_orig:
> 	kfree(orig_data);
> +	kfree(s_mount_opts);
> 	return err ? err : ret;
> }


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] ext4: use more strict checks for inodes_per_block on mount

[Andreas Dilger]

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

On Nov 17, 2016, at 9:26 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> Centralize the checks for inodes_per_block and be more strict to make
> sure the inodes_per_block_group can't end up being zero.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> Cc: stable@vger.kernel.org
> ---
> fs/ext4/super.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3009c03220d6..6638a0415997 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3666,12 +3666,16 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 
> 	sbi->s_blocks_per_group = le32_to_cpu(es->s_blocks_per_group);
> 	sbi->s_inodes_per_group = le32_to_cpu(es->s_inodes_per_group);
> -	if (EXT4_INODE_SIZE(sb) == 0 || EXT4_INODES_PER_GROUP(sb) == 0)
> -		goto cantfind_ext4;
> 
> 	sbi->s_inodes_per_block = blocksize / EXT4_INODE_SIZE(sb);
> 	if (sbi->s_inodes_per_block == 0)
> 		goto cantfind_ext4;
> +	if (sbi->s_inodes_per_group < sbi->s_inodes_per_block ||
> +	    sbi->s_inodes_per_group > blocksize * 8) {
> +		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
> +			 sbi->s_blocks_per_group);
> +		goto failed_mount;
> +	}
> 	sbi->s_itb_per_group = sbi->s_inodes_per_group /
> 					sbi->s_inodes_per_block;
> 	sbi->s_desc_per_block = blocksize / EXT4_DESC_SIZE(sb);
> @@ -3754,13 +3758,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	}
> 	sbi->s_cluster_ratio = clustersize / blocksize;
> 
> -	if (sbi->s_inodes_per_group > blocksize * 8) {
> -		ext4_msg(sb, KERN_ERR,
> -		       "#inodes per group too big: %lu",
> -		       sbi->s_inodes_per_group);
> -		goto failed_mount;
> -	}
> -
> 	/* Do we have standard group size of clustersize * 8 blocks ? */
> 	if (sbi->s_blocks_per_group == clustersize << 3)
> 		set_opt2(sb, STD_GROUP_SIZE);
> --
> 2.11.0.rc0.7.gbe5a750
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

On Fri, Nov 18, 2016 at 09:40:02AM +0200, Nikolay Borisov wrote:
> 
> 
> On 11/18/2016 06:26 AM, Theodore Ts'o wrote:
> > If the block size or cluster size is insane, reject the mount.  This
> > is important for security reasons (although we shouldn't be just
> > depending on this check).
> > 
> > Ref: http://www.securityfocus.com/archive/1/539661
> > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1332506
> > Reported-by: Borislav Petkov <bp@alien8.de>
> > Reported-by: Nikolay Borisov <kernel@kyup.com>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Cc: stable@vger.kernel.org
> 
> I just tested with the 3 patches applied on 4.9-rc4 and could still
> reproduce the issue.

I've just tested the dev branch on both x86 and x86_64 and it doesn't reproduce for me;

root@xfstests:~# mount -o ro,loop /tmp/OSS-2016-23-image  /mnt
mount: wrong fs type, bad option, bad superblock on /dev/loop0,
       missing codepage or helper program, or other error

       In some cases useful info is found in syslog - try
       dmesg | tail or so.
root@xfstests:~# dmesg | tail -3
[  910.472554] EXT4-fs (loop0): Unrecognized mount option "" or missing value
[  910.479708] EXT4-fs (loop0): failed to parse options in superblock: 
[  910.486279] EXT4-fs (loop0): Invalid log block size: 4286906368
root@xfstests:~# 

The critical patch should be:

    ext4: sanity check the block and cluster size at mount time

Can you double check your test?

Thanks,

						- Ted

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

On Fri, Nov 18, 2016 at 01:38:10AM -0700, Andreas Dilger wrote:
> > @@ -3567,7 +3567,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > 	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
> > 	    blocksize > EXT4_MAX_BLOCK_SIZE) {
> > 		ext4_msg(sb, KERN_ERR,
> > -		       "Unsupported filesystem blocksize %d", blocksize);
> > +		       "Unsupported filesystem blocksize %d (%d)",
> 
> Would be good to indicate what the second value is, like "(%d bits)".

Good idea; I've annotated it as "log_block_size".

					- Ted

Re: [PATCH 2/3] ext4: fix in-superblock mount options processing

[Theodore Ts'o]

On Fri, Nov 18, 2016 at 02:38:41AM -0700, Andreas Dilger wrote:
> > @@ -3321,17 +3321,17 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > 	int err = 0;
> > 	unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> > 	ext4_group_t first_not_zeroed;
> > +	char *s_mount_opts = kzalloc(sizeof(sbi->s_es->s_mount_opts)+1,
> > +				   GFP_KERNEL);
> 
> This doesn't check if "sbi" is non-NULL before dereferencing it.  While
> there isn't really any likelihood for this allocation to fail, I don't
> think there is any benefit to moving these allocations to the declaration.

This should be safe because sizeof(sbi->s_es->s_mount_opts) is a
constant, so it would be calculated at compile time.  You're right
that we can use kstrndup() below, so I'll do that, though, in which
case s_mount_opts can be initialized to NULL.

> > +	if (sbi->s_es->s_mount_opts[0]) {
> > +		strncpy(s_mount_opts, sbi->s_es->s_mount_opts,
> > +			sizeof(sbi->s_es->s_mount_opts));
> 
> This could use kstrndup()?  It always allocates an extra byte for the NUL.

Will do.

> > @@ -4162,7 +4166,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > 
> > 	if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount"))
> > 		ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> > -			 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> > +			 "Opts: %.*s%s%s", descr,
> > +			 sizeof(sbi->s_es->s_mount_opts),
> > +			 sbi->s_es->s_mount_opts,
> 
> This isn't really needed, since there is always a NUL terminator added
> to the string?

This is sbi->s_es->s_mount_opts, not mount_opts.  And the former is
not necessarily guaranteed to be NUL terminated.  And the reason why
we can't use mount_opts is because parse_options() destroys the
passed-in string as part of the parsing.

Cheers,

						- Ted

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Nikolay Borisov]

On Fri, Nov 18, 2016 at 7:58 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Fri, Nov 18, 2016 at 09:40:02AM +0200, Nikolay Borisov wrote:
>>
>>
>> On 11/18/2016 06:26 AM, Theodore Ts'o wrote:
>> > If the block size or cluster size is insane, reject the mount.  This
>> > is important for security reasons (although we shouldn't be just
>> > depending on this check).
>> >
>> > Ref: http://www.securityfocus.com/archive/1/539661
>> > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1332506
>> > Reported-by: Borislav Petkov <bp@alien8.de>
>> > Reported-by: Nikolay Borisov <kernel@kyup.com>
>> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>> > Cc: stable@vger.kernel.org
>>
>> I just tested with the 3 patches applied on 4.9-rc4 and could still
>> reproduce the issue.
>
> I've just tested the dev branch on both x86 and x86_64 and it doesn't reproduce for me;

As per my original email in reporting this issue - there was someone
whose 4.9-rc4 kernel + some tip/ stuff, I believe, wasn't crashing.
And the error message printed by mount was the same as yours. What I
suspect is happening is that some other patch in ext4 is catching
earlier invalid data and aborts the mount before we can crash due to
the issue at hand.

>
> root@xfstests:~# mount -o ro,loop /tmp/OSS-2016-23-image  /mnt
> mount: wrong fs type, bad option, bad superblock on /dev/loop0,
>        missing codepage or helper program, or other error
>
>        In some cases useful info is found in syslog - try
>        dmesg | tail or so.
> root@xfstests:~# dmesg | tail -3
> [  910.472554] EXT4-fs (loop0): Unrecognized mount option "" or missing value
> [  910.479708] EXT4-fs (loop0): failed to parse options in superblock:
> [  910.486279] EXT4-fs (loop0): Invalid log block size: 4286906368
> root@xfstests:~#
>
> The critical patch should be:
>
>     ext4: sanity check the block and cluster size at mount time
>
> Can you double check your test?

I will. And as I said i'm testing 4.9-rc4 from Linus' tree with only
those patches applied.


>
> Thanks,
>
>                                                 - Ted

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

On Sat, Nov 19, 2016 at 01:25:58AM +0200, Nikolay Borisov wrote:
> > Can you double check your test?
> 
> I will. And as I said i'm testing 4.9-rc4 from Linus' tree with only
> those patches applied.
> 

I just tested v4.9-rc4 with just the critical patch.  See the test-fix
branch on ext4.git on kernel.org:

% git log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit -2
* e7d56cc34f5d - (HEAD -> test-fix) ext4: sanity check the block and cluster size at mount time (13 minutes ago)
* bc33b0ca11e3 - (tag: v4.9-rc4) Linux 4.9-rc4 (13 days ago)

It's not reproing for me:

% kvm-xfstests shell
Networking disabled.
[    5.202323] systemd-fsck[1353]: /dev/vda: clean, 10882/65536 files, 73964/262144 blocks
[    5.645642] systemd-fsck[2397]: /dev/vdg: recovering journal
[    5.660848] systemd-fsck[2397]: /dev/vdg: clean, 50/65536 files, 12993/262144 blocks

Debian GNU/Linux 8 kvm-xfstests ttyS0

kvm-xfstests login: root (automatic login)

Last login: Fri Nov 18 21:34:51 EST 2016 on ttyS3
uLinux kvm-xfstests 4.9.0-rc4-00001-ge7d56cc34f5d #66 SMP Fri Nov 18 21:25:00 EST 2016 i686

The programs included with the Debian GNU/Linux system are free software;
the exact distribution terms for each program are described in the
individual files in /usr/share/doc/*/copyright.

Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
permitted by applicable law.
root@kvm-xfstests:~# uname -a
Linux kvm-xfstests 4.9.0-rc4-00001-ge7d56cc34f5d #66 SMP Fri Nov 18 21:25:00 EST 2016 i686 GNU/Linux
root@kvm-xfstests:~# dmesg -n 7
root@kvm-xfstests:~# mount /vdb
[   13.241090] EXT4-fs (vdb): recovery complete
[   13.250367] EXT4-fs (vdb): mounted filesystem with ordered data mode. Opts: (null)
root@kvm-xfstests:~# md5sum /vdb/OSS-2016-23-image 
f05e467ff6674a6b469a3b86c972dafa  /vdb/OSS-2016-23-image
root@kvm-xfstests:~# mount -o loop,ro /vdb/OSS-2016-23-image /mnt
[   36.160416] EXT4-fs (loop0): Unrecognized mount option "" or missing value
[   36.170298] EXT4-fs (loop0): failed to parse options in superblock: 
[   36.177737] EXT4-fs: Warning: mounting with data=journal disables delayed allocation and O_DIRECT support!
[   36.183217] EXT4-fs (loop0): Invalid log block size: 4286906368
mount: wrong fs type, bad option, bad superblock on /dev/loop0,
       missing codepage or helper program, or other error

       In some cases useful info is found in syslog - try
       dmesg | tail or so.
root@kvm-xfstests:~# QEMU: Terminated

							- Ted

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Nikolay Borisov]

On 11/19/2016 04:36 AM, Theodore Ts'o wrote:
> On Sat, Nov 19, 2016 at 01:25:58AM +0200, Nikolay Borisov wrote:
>>> Can you double check your test?
>>
>> I will. And as I said i'm testing 4.9-rc4 from Linus' tree with only
>> those patches applied.
>>
> 
> I just tested v4.9-rc4 with just the critical patch.  See the test-fix
> branch on ext4.git on kernel.org:
> 
> % git log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit -2
> * e7d56cc34f5d - (HEAD -> test-fix) ext4: sanity check the block and cluster size at mount time (13 minutes ago)
> * bc33b0ca11e3 - (tag: v4.9-rc4) Linux 4.9-rc4 (13 days ago)
> 
> It's not reproing for me:

So I downloaded the image again and tested and it indeed doesn't reproduce. However, testing with the same image, but downloaded 
couple of days ago I can still reproduce this. The checksums of both images differ though. The freshly downloaded has the same 
checksum as your whereas my earlier download of it doesn't. All of this without 4/4 being applied. 

With patch 4/4 applied I can actually mount my "earlier" image which was causing a crash even with your other patches applied: 


[root@localhost /]# uname -a
Linux localhost 4.9.0-rc4-clouder1 #47 SMP Mon Nov 21 09:28:17 EET 2016 x86_64 x86_64 x86_64 GNU/Linux

[root@localhost /]# md5sum root/OSS-2016-22-image 
0a53a280ed976d7416b5af0fdd543e0f  root/OSS-2016-22-image

mount -o loop root/OSS-2016-22-image root/ovl-mnt/
dmesg
[  125.376228] EXT4-fs (loop0): filesystem is read-only
[  125.376513] EXT4-fs (loop0): ext4_check_descriptors: Inode bitmap for group 0 overlaps superblock
[  125.376768] EXT4-fs (loop0): ext4_check_descriptors: Checksum for group 0 failed (25303!=248)
[  125.377059] EXT4-fs error (device loop0): count_overhead:3199: comm mount: Invalid number of block group descriptor blocks: 842150400
[  125.381546] EXT4-fs (loop0): revision level too high, forcing read-only mode
[  125.381733] [EXT4 FS bs=1024, gc=1, bpg=8192, ipg=16, mo=e800c02c, mo2=0002]
[  125.381889] System zones: 0-842150400
[  125.382656] EXT4-fs (loop0): filesystem is read-only
[  125.382805] EXT4-fs (loop0): mounted filesystem without journal. Opts: (null)

[root@localhost /]# losetup -a    
/dev/loop0: [fd00]:9518 (/root/OSS-2016-22-image)

So patch 4/4 is also a good precaution. I'd say the issue is resolved. 

Tested-by: Nikolay Borisov <kernel@kyup.com>



> 
> % kvm-xfstests shell
> Networking disabled.
> [    5.202323] systemd-fsck[1353]: /dev/vda: clean, 10882/65536 files, 73964/262144 blocks
> [    5.645642] systemd-fsck[2397]: /dev/vdg: recovering journal
> [    5.660848] systemd-fsck[2397]: /dev/vdg: clean, 50/65536 files, 12993/262144 blocks
> 
> Debian GNU/Linux 8 kvm-xfstests ttyS0
> 
> kvm-xfstests login: root (automatic login)
> 
> Last login: Fri Nov 18 21:34:51 EST 2016 on ttyS3
> uLinux kvm-xfstests 4.9.0-rc4-00001-ge7d56cc34f5d #66 SMP Fri Nov 18 21:25:00 EST 2016 i686
> 
> The programs included with the Debian GNU/Linux system are free software;
> the exact distribution terms for each program are described in the
> individual files in /usr/share/doc/*/copyright.
> 
> Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
> permitted by applicable law.
> root@kvm-xfstests:~# uname -a
> Linux kvm-xfstests 4.9.0-rc4-00001-ge7d56cc34f5d #66 SMP Fri Nov 18 21:25:00 EST 2016 i686 GNU/Linux
> root@kvm-xfstests:~# dmesg -n 7
> root@kvm-xfstests:~# mount /vdb
> [   13.241090] EXT4-fs (vdb): recovery complete
> [   13.250367] EXT4-fs (vdb): mounted filesystem with ordered data mode. Opts: (null)
> root@kvm-xfstests:~# md5sum /vdb/OSS-2016-23-image 
> f05e467ff6674a6b469a3b86c972dafa  /vdb/OSS-2016-23-image
> root@kvm-xfstests:~# mount -o loop,ro /vdb/OSS-2016-23-image /mnt
> [   36.160416] EXT4-fs (loop0): Unrecognized mount option "" or missing value
> [   36.170298] EXT4-fs (loop0): failed to parse options in superblock: 
> [   36.177737] EXT4-fs: Warning: mounting with data=journal disables delayed allocation and O_DIRECT support!
> [   36.183217] EXT4-fs (loop0): Invalid log block size: 4286906368
> mount: wrong fs type, bad option, bad superblock on /dev/loop0,
>        missing codepage or helper program, or other error
> 
>        In some cases useful info is found in syslog - try
>        dmesg | tail or so.
> root@kvm-xfstests:~# QEMU: Terminated
> 
> 							- Ted
> 

Re: [PATCH 1/3] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

On Mon, Nov 21, 2016 at 09:46:45AM +0200, Nikolay Borisov wrote:
> 
> So I downloaded the image again and tested and it indeed doesn't
> reproduce. However, testing with the same image, but downloaded
> couple of days ago I can still reproduce this. The checksums of both
> images differ though. The freshly downloaded has the same checksum
> as your whereas my earlier download of it doesn't. All of this
> without 4/4 being applied.
> 
> With patch 4/4 applied I can actually mount my "earlier" image which
> was causing a crash even with your other patches applied:
> 

Just for the record, can you send me a copy of the older image?  I
just want to be sure I understand what was going on.

Thanks!!

					- Ted