* crypto: Kernel memory overwrite attempt detected to spans multiple pages @ 2019-03-19 11:54 Geert Uytterhoeven 2019-03-19 17:09 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Geert Uytterhoeven @ 2019-03-19 11:54 UTC (permalink / raw) To: Kees Cook, Herbert Xu Cc: linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List When running the sha1-asm crypto selftest on arm with CONFIG_HARDENED_USERCOPY_PAGESPAN=y: usercopy: Kernel memory overwrite attempt detected to spans multiple pages (offset 0, size 42)! ------------[ cut here ]------------ kernel BUG at mm/usercopy.c:102! Internal error: Oops - BUG: 0 [#1] SMP ARM Modules linked in: CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 Hardware name: Generic R-Car Gen2 (Flattened Device Tree) PC is at usercopy_abort+0x68/0x90 LR is at usercopy_abort+0x68/0x90 pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013 sp : ea54bc60 ip : 00000010 fp : cccccccd r10: 00000000 r9 : c0e0ce04 r8 : ea54d009 r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120 r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 30c5387d Table: 40003000 DAC: fffffffd Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval)) Stack: (0xea54bc60 to 0xea54c000) bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a 00000000 c0310060 bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8 ea54cfe0 ea538c00 bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84 0000003a c0427a30 bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70 0000002a c081cf80 bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8 c0e0a408 eb074240 bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422 ebef7480 eb074200 bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014 c084061c c0428c38 bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40 00000002 eabe4e80 bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060 00000006 c09d544c bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001 eb074200 00000000 bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0 00000000 c081cf70 bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780 c0e4ee80 443e9884 bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe fefefefe fefefefe be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298 ea4f7a00 c03062bc be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00 00000002 eabe4e40 be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314 eb074200 ea538c00 be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006 c04dace8 00000006 be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08 ea4f71a8 c0429420 bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374 00000400 ffffffff bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438 c0e5a2a0 c0e5a2a0 bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013 c0e5a2a0 c0269470 bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408 ea537280 0000000e bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4 c09c9ed0 ea54bf5c bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422 c09c9ed0 ea537200 bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08 ea4f71a8 c0426524 bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000 00000000 00000000 bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [<c030fd60>] (usercopy_abort) from [<c0310060>] (__check_object_size+0x2d8/0x448) [<c0310060>] (__check_object_size) from [<c0427a30>] (build_test_sglist+0x268/0x2d8) [<c0427a30>] (build_test_sglist) from [<c0428c38>] (test_hash_vec_cfg+0x110/0x694) [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] (__alg_test_hash+0x158/0x1b8) [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) Exception stack(0xea54bfb0 to 0xea54bff8) bfa0: 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2) ---[ end trace 190b3cf48e720f78 ]--- BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:34 in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 Hardware name: Generic R-Car Gen2 (Flattened Device Tree) [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14) [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c) [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158) [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258) [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20) [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344) [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190) [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c) Exception stack(0xea54bc10 to 0xea54bc58) bc00: 0000005f 2abb4000 dd6cd422 dd6cd422 bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04 00000000 cccccccd bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90) [<c030fd60>] (usercopy_abort) from [<c0310060>] (__check_object_size+0x2d8/0x448) [<c0310060>] (__check_object_size) from [<c0427a30>] (build_test_sglist+0x268/0x2d8) [<c0427a30>] (build_test_sglist) from [<c0428c38>] (test_hash_vec_cfg+0x110/0x694) [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] (__alg_test_hash+0x158/0x1b8) [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) Exception stack(0xea54bfb0 to 0xea54bff8) bfa0: 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 A similar trace is seen with sha1-ce on arm64: usercopy: Kernel memory overwrite attempt detected to spans multiple pages (offset 0, size 42)! ------------[ cut here ]------------ kernel BUG at mm/usercopy.c:102! Internal error: Oops - BUG: 0 [#1] SMP Modules linked in: CPU: 1 PID: 33 Comm: cryptomgr_test Not tainted 5.1.0-rc1-salvator-x-01109-gbeb7d6376ecfbf07-dirty #352 Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT) pstate: 60400005 (nZCv daif +PAN -UAO) pc : usercopy_abort+0x64/0x90 lr : usercopy_abort+0x64/0x90 sp : ffffff8011eb38d0 x29: ffffff8011eb38e0 x28: 6db6db6db6db6db7 x27: ffffffbf00000000 x26: 0000000000000038 x25: ffffffc0778fd009 x24: 0000000000000000 x23: ffffffc0778fd00a x22: ffffff8010d51000 x21: 0000000000000000 x20: 000000000000002a x19: ffffffc0778fcfe0 x18: 000000000000000a x17: 00000000526a1be5 x16: 0000000000000014 x15: 000000000009f6c2 x14: 0720072007200720 x13: 0720072007200720 x12: 0720072007200720 x11: 0720072007200720 x10: 0720072007200720 x9 : ffffff80110126c8 x8 : 0000000000000000 x7 : ffffff801015700c x6 : 0000000000000000 x5 : 0000000000000000 x4 : ffffff8011eb4000 x3 : 0000000000000080 x2 : a045404094166600 x1 : 0000000000000000 x0 : 000000000000005f Process cryptomgr_test (pid: 33, stack limit = 0x(____ptrval____)) Call trace: usercopy_abort+0x64/0x90 __check_object_size+0x64/0x464 build_test_sglist+0x238/0x2c8 test_hash_vec_cfg+0x130/0x660 __alg_test_hash+0x1b4/0x1f4 alg_test_hash+0x88/0x104 alg_test.part.6+0x2a8/0x330 alg_test+0x98/0xa0 cryptomgr_test+0x24/0x4c kthread+0x120/0x130 ret_from_fork+0x10/0x18 Code: aa0003e3 b00053e0 91148000 97fc3bf2 (d4210000) ---[ end trace d9f3261d50a7f84f ]--- BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:34 in_atomic(): 0, irqs_disabled(): 128, pid: 33, name: cryptomgr_test INFO: lockdep is turned off. irq event stamp: 262 hardirqs last enabled at (261): [<ffffff8010157050>] console_unlock+0x554/0x560 hardirqs last disabled at (262): [<ffffff8010081a28>] do_debug_exception+0x48/0x13c softirqs last enabled at (258): [<ffffff8010081ee4>] __do_softirq+0x18c/0x4a0 softirqs last disabled at (245): [<ffffff80100f3e10>] irq_exit+0xa4/0x100 CPU: 1 PID: 33 Comm: cryptomgr_test Tainted: G D 5.1.0-rc1-salvator-x-01109-gbeb7d6376ecfbf07-dirty #352 Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT) Call trace: dump_backtrace+0x0/0x118 show_stack+0x14/0x1c dump_stack+0xc8/0x118 ___might_sleep+0x24c/0x25c __might_sleep+0x70/0x80 exit_signals+0x48/0x278 do_exit+0x10c/0xa30 die+0x1f4/0x208 bug_handler+0x4c/0x78 brk_handler+0x15c/0x188 do_debug_exception+0xd4/0x13c el1_dbg+0x18/0xbc usercopy_abort+0x64/0x90 __check_object_size+0x64/0x464 build_test_sglist+0x238/0x2c8 test_hash_vec_cfg+0x130/0x660 __alg_test_hash+0x1b4/0x1f4 alg_test_hash+0x88/0x104 alg_test.part.6+0x2a8/0x330 alg_test+0x98/0xa0 cryptomgr_test+0x24/0x4c kthread+0x120/0x130 ret_from_fork+0x10/0x18 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-03-19 11:54 crypto: Kernel memory overwrite attempt detected to spans multiple pages Geert Uytterhoeven @ 2019-03-19 17:09 ` Eric Biggers 2019-03-20 18:57 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-03-19 17:09 UTC (permalink / raw) To: Geert Uytterhoeven, Kees Cook Cc: Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > When running the sha1-asm crypto selftest on arm with > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > usercopy: Kernel memory overwrite attempt detected to spans > multiple pages (offset 0, size 42)! > ------------[ cut here ]------------ > kernel BUG at mm/usercopy.c:102! > Internal error: Oops - BUG: 0 [#1] SMP ARM > Modules linked in: > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > PC is at usercopy_abort+0x68/0x90 > LR is at usercopy_abort+0x68/0x90 > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013 > sp : ea54bc60 ip : 00000010 fp : cccccccd > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009 > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120 > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > Control: 30c5387d Table: 40003000 DAC: fffffffd > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval)) > Stack: (0xea54bc60 to 0xea54c000) > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a > 00000000 c0310060 > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8 > ea54cfe0 ea538c00 > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84 > 0000003a c0427a30 > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70 > 0000002a c081cf80 > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8 > c0e0a408 eb074240 > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422 > ebef7480 eb074200 > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014 > c084061c c0428c38 > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40 > 00000002 eabe4e80 > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060 > 00000006 c09d544c > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001 > eb074200 00000000 > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0 > 00000000 c081cf70 > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780 > c0e4ee80 443e9884 > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe > fefefefe fefefefe > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298 > ea4f7a00 c03062bc > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00 > 00000002 eabe4e40 > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314 > eb074200 ea538c00 > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006 > c04dace8 00000006 > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08 > ea4f71a8 c0429420 > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374 > 00000400 ffffffff > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438 > c0e5a2a0 c0e5a2a0 > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013 > c0e5a2a0 c0269470 > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408 > ea537280 0000000e > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4 > c09c9ed0 ea54bf5c > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422 > c09c9ed0 ea537200 > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08 > ea4f71a8 c0426524 > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000 > 00000000 00000000 > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000 > 00000000 00000000 > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 00000000 > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > 00000000 00000000 > [<c030fd60>] (usercopy_abort) from [<c0310060>] > (__check_object_size+0x2d8/0x448) > [<c0310060>] (__check_object_size) from [<c0427a30>] > (build_test_sglist+0x268/0x2d8) > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > (test_hash_vec_cfg+0x110/0x694) > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > (__alg_test_hash+0x158/0x1b8) > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > Exception stack(0xea54bfb0 to 0xea54bff8) > bfa0: 00000000 00000000 > 00000000 00000000 > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 00000000 > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2) > ---[ end trace 190b3cf48e720f78 ]--- > BUG: sleeping function called from invalid context at > include/linux/percpu-rwsem.h:34 > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14) > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c) > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158) > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258) > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20) > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344) > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190) > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c) > Exception stack(0xea54bc10 to 0xea54bc58) > bc00: 0000005f 2abb4000 > dd6cd422 dd6cd422 > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04 > 00000000 cccccccd > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90) > [<c030fd60>] (usercopy_abort) from [<c0310060>] > (__check_object_size+0x2d8/0x448) > [<c0310060>] (__check_object_size) from [<c0427a30>] > (build_test_sglist+0x268/0x2d8) > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > (test_hash_vec_cfg+0x110/0x694) > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > (__alg_test_hash+0x158/0x1b8) > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > Exception stack(0xea54bfb0 to 0xea54bff8) > bfa0: 00000000 00000000 > 00000000 00000000 > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 00000000 > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > A similar trace is seen with sha1-ce on arm64: > > usercopy: Kernel memory overwrite attempt detected to spans > multiple pages (offset 0, size 42)! > ------------[ cut here ]------------ > kernel BUG at mm/usercopy.c:102! > Internal error: Oops - BUG: 0 [#1] SMP > Modules linked in: > CPU: 1 PID: 33 Comm: cryptomgr_test Not tainted > 5.1.0-rc1-salvator-x-01109-gbeb7d6376ecfbf07-dirty #352 > Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT) > pstate: 60400005 (nZCv daif +PAN -UAO) > pc : usercopy_abort+0x64/0x90 > lr : usercopy_abort+0x64/0x90 > sp : ffffff8011eb38d0 > x29: ffffff8011eb38e0 x28: 6db6db6db6db6db7 > x27: ffffffbf00000000 x26: 0000000000000038 > x25: ffffffc0778fd009 x24: 0000000000000000 > x23: ffffffc0778fd00a x22: ffffff8010d51000 > x21: 0000000000000000 x20: 000000000000002a > x19: ffffffc0778fcfe0 x18: 000000000000000a > x17: 00000000526a1be5 x16: 0000000000000014 > x15: 000000000009f6c2 x14: 0720072007200720 > x13: 0720072007200720 x12: 0720072007200720 > x11: 0720072007200720 x10: 0720072007200720 > x9 : ffffff80110126c8 x8 : 0000000000000000 > x7 : ffffff801015700c x6 : 0000000000000000 > x5 : 0000000000000000 x4 : ffffff8011eb4000 > x3 : 0000000000000080 x2 : a045404094166600 > x1 : 0000000000000000 x0 : 000000000000005f > Process cryptomgr_test (pid: 33, stack limit = 0x(____ptrval____)) > Call trace: > usercopy_abort+0x64/0x90 > __check_object_size+0x64/0x464 > build_test_sglist+0x238/0x2c8 > test_hash_vec_cfg+0x130/0x660 > __alg_test_hash+0x1b4/0x1f4 > alg_test_hash+0x88/0x104 > alg_test.part.6+0x2a8/0x330 > alg_test+0x98/0xa0 > cryptomgr_test+0x24/0x4c > kthread+0x120/0x130 > ret_from_fork+0x10/0x18 > Code: aa0003e3 b00053e0 91148000 97fc3bf2 (d4210000) > ---[ end trace d9f3261d50a7f84f ]--- > BUG: sleeping function called from invalid context at > include/linux/percpu-rwsem.h:34 > in_atomic(): 0, irqs_disabled(): 128, pid: 33, name: cryptomgr_test > INFO: lockdep is turned off. > irq event stamp: 262 > hardirqs last enabled at (261): [<ffffff8010157050>] > console_unlock+0x554/0x560 > hardirqs last disabled at (262): [<ffffff8010081a28>] > do_debug_exception+0x48/0x13c > softirqs last enabled at (258): [<ffffff8010081ee4>] > __do_softirq+0x18c/0x4a0 > softirqs last disabled at (245): [<ffffff80100f3e10>] irq_exit+0xa4/0x100 > CPU: 1 PID: 33 Comm: cryptomgr_test Tainted: G D > 5.1.0-rc1-salvator-x-01109-gbeb7d6376ecfbf07-dirty #352 > Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT) > Call trace: > dump_backtrace+0x0/0x118 > show_stack+0x14/0x1c > dump_stack+0xc8/0x118 > ___might_sleep+0x24c/0x25c > __might_sleep+0x70/0x80 > exit_signals+0x48/0x278 > do_exit+0x10c/0xa30 > die+0x1f4/0x208 > bug_handler+0x4c/0x78 > brk_handler+0x15c/0x188 > do_debug_exception+0xd4/0x13c > el1_dbg+0x18/0xbc > usercopy_abort+0x64/0x90 > __check_object_size+0x64/0x464 > build_test_sglist+0x238/0x2c8 > test_hash_vec_cfg+0x130/0x660 > __alg_test_hash+0x1b4/0x1f4 > alg_test_hash+0x88/0x104 > alg_test.part.6+0x2a8/0x330 > alg_test+0x98/0xa0 > cryptomgr_test+0x24/0x4c > kthread+0x120/0x130 > ret_from_fork+0x10/0x18 > > Gr{oetje,eeting}s, > > Geert > Well, this must happen with the new (in 5.1) crypto self-tests implementation for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't understand why hardened usercopy considers it a bug though, as there's no buffer overflow. The crypto tests use copy_from_iter() to copy data into a 2-page buffer that was allocated with __get_free_pages(): __get_free_pages(GFP_KERNEL, 1) ... where 1 means an order-1 allocation. If it copies to offset=4064 len=42, for example, then hardened usercopy considers it a bug even though the buffer is 8192 bytes long. Why? It isn't actually copying anything to/from userspace, BTW; it's using iov_iter with ITER_KVEC. - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-03-19 17:09 ` Eric Biggers @ 2019-03-20 18:57 ` Eric Biggers 2019-03-21 17:45 ` Kees Cook 0 siblings, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-03-20 18:57 UTC (permalink / raw) To: Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote: > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > > When running the sha1-asm crypto selftest on arm with > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > > > usercopy: Kernel memory overwrite attempt detected to spans > > multiple pages (offset 0, size 42)! > > ------------[ cut here ]------------ > > kernel BUG at mm/usercopy.c:102! > > Internal error: Oops - BUG: 0 [#1] SMP ARM > > Modules linked in: > > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > PC is at usercopy_abort+0x68/0x90 > > LR is at usercopy_abort+0x68/0x90 > > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013 > > sp : ea54bc60 ip : 00000010 fp : cccccccd > > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009 > > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120 > > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > > Control: 30c5387d Table: 40003000 DAC: fffffffd > > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval)) > > Stack: (0xea54bc60 to 0xea54c000) > > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a > > 00000000 c0310060 > > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8 > > ea54cfe0 ea538c00 > > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84 > > 0000003a c0427a30 > > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70 > > 0000002a c081cf80 > > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8 > > c0e0a408 eb074240 > > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422 > > ebef7480 eb074200 > > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014 > > c084061c c0428c38 > > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40 > > 00000002 eabe4e80 > > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060 > > 00000006 c09d544c > > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001 > > eb074200 00000000 > > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0 > > 00000000 c081cf70 > > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780 > > c0e4ee80 443e9884 > > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe > > fefefefe fefefefe > > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298 > > ea4f7a00 c03062bc > > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00 > > 00000002 eabe4e40 > > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314 > > eb074200 ea538c00 > > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006 > > c04dace8 00000006 > > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08 > > ea4f71a8 c0429420 > > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374 > > 00000400 ffffffff > > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438 > > c0e5a2a0 c0e5a2a0 > > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013 > > c0e5a2a0 c0269470 > > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408 > > ea537280 0000000e > > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4 > > c09c9ed0 ea54bf5c > > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422 > > c09c9ed0 ea537200 > > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08 > > ea4f71a8 c0426524 > > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000 > > 00000000 00000000 > > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000 > > 00000000 00000000 > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > 00000000 00000000 > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > 00000000 00000000 > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > (__check_object_size+0x2d8/0x448) > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > (build_test_sglist+0x268/0x2d8) > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > (test_hash_vec_cfg+0x110/0x694) > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > (__alg_test_hash+0x158/0x1b8) > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > Exception stack(0xea54bfb0 to 0xea54bff8) > > bfa0: 00000000 00000000 > > 00000000 00000000 > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > 00000000 00000000 > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2) > > ---[ end trace 190b3cf48e720f78 ]--- > > BUG: sleeping function called from invalid context at > > include/linux/percpu-rwsem.h:34 > > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test > > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14) > > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c) > > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158) > > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258) > > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20) > > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344) > > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190) > > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c) > > Exception stack(0xea54bc10 to 0xea54bc58) > > bc00: 0000005f 2abb4000 > > dd6cd422 dd6cd422 > > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04 > > 00000000 cccccccd > > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff > > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90) > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > (__check_object_size+0x2d8/0x448) > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > (build_test_sglist+0x268/0x2d8) > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > (test_hash_vec_cfg+0x110/0x694) > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > (__alg_test_hash+0x158/0x1b8) > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > Exception stack(0xea54bfb0 to 0xea54bff8) > > bfa0: 00000000 00000000 > > 00000000 00000000 > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > 00000000 00000000 > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't > understand why hardened usercopy considers it a bug though, as there's no buffer > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page > buffer that was allocated with __get_free_pages(): > > __get_free_pages(GFP_KERNEL, 1) > > ... where 1 means an order-1 allocation. > > If it copies to offset=4064 len=42, for example, then hardened usercopy > considers it a bug even though the buffer is 8192 bytes long. Why? > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter > with ITER_KVEC. > > - Eric Kees, any thoughts on why hardened usercopy rejects copies spanning a page boundary when they seem to be fine? - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-03-20 18:57 ` Eric Biggers @ 2019-03-21 17:45 ` Kees Cook 2019-03-21 17:51 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-03-21 17:45 UTC (permalink / raw) To: Eric Biggers Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote: > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > > > When running the sha1-asm crypto selftest on arm with > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > > > > > usercopy: Kernel memory overwrite attempt detected to spans > > > multiple pages (offset 0, size 42)! > > > ------------[ cut here ]------------ > > > kernel BUG at mm/usercopy.c:102! > > > Internal error: Oops - BUG: 0 [#1] SMP ARM > > > Modules linked in: > > > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > > PC is at usercopy_abort+0x68/0x90 > > > LR is at usercopy_abort+0x68/0x90 > > > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013 > > > sp : ea54bc60 ip : 00000010 fp : cccccccd > > > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009 > > > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120 > > > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f > > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > > > Control: 30c5387d Table: 40003000 DAC: fffffffd > > > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval)) > > > Stack: (0xea54bc60 to 0xea54c000) > > > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a > > > 00000000 c0310060 > > > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8 > > > ea54cfe0 ea538c00 > > > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84 > > > 0000003a c0427a30 > > > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70 > > > 0000002a c081cf80 > > > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8 > > > c0e0a408 eb074240 > > > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422 > > > ebef7480 eb074200 > > > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014 > > > c084061c c0428c38 > > > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40 > > > 00000002 eabe4e80 > > > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060 > > > 00000006 c09d544c > > > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001 > > > eb074200 00000000 > > > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0 > > > 00000000 c081cf70 > > > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780 > > > c0e4ee80 443e9884 > > > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe > > > fefefefe fefefefe > > > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298 > > > ea4f7a00 c03062bc > > > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00 > > > 00000002 eabe4e40 > > > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314 > > > eb074200 ea538c00 > > > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006 > > > c04dace8 00000006 > > > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08 > > > ea4f71a8 c0429420 > > > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374 > > > 00000400 ffffffff > > > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438 > > > c0e5a2a0 c0e5a2a0 > > > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013 > > > c0e5a2a0 c0269470 > > > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408 > > > ea537280 0000000e > > > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4 > > > c09c9ed0 ea54bf5c > > > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422 > > > c09c9ed0 ea537200 > > > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08 > > > ea4f71a8 c0426524 > > > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000 > > > 00000000 00000000 > > > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000 > > > 00000000 00000000 > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > 00000000 00000000 > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > 00000000 00000000 > > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > > (__check_object_size+0x2d8/0x448) > > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > > (build_test_sglist+0x268/0x2d8) > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > > (test_hash_vec_cfg+0x110/0x694) > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > > (__alg_test_hash+0x158/0x1b8) > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > > Exception stack(0xea54bfb0 to 0xea54bff8) > > > bfa0: 00000000 00000000 > > > 00000000 00000000 > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > 00000000 00000000 > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2) > > > ---[ end trace 190b3cf48e720f78 ]--- > > > BUG: sleeping function called from invalid context at > > > include/linux/percpu-rwsem.h:34 > > > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test > > > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14) > > > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c) > > > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158) > > > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258) > > > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20) > > > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344) > > > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190) > > > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c) > > > Exception stack(0xea54bc10 to 0xea54bc58) > > > bc00: 0000005f 2abb4000 > > > dd6cd422 dd6cd422 > > > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04 > > > 00000000 cccccccd > > > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff > > > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90) > > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > > (__check_object_size+0x2d8/0x448) > > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > > (build_test_sglist+0x268/0x2d8) > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > > (test_hash_vec_cfg+0x110/0x694) > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > > (__alg_test_hash+0x158/0x1b8) > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > > Exception stack(0xea54bfb0 to 0xea54bff8) > > > bfa0: 00000000 00000000 > > > 00000000 00000000 > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > 00000000 00000000 > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't > > understand why hardened usercopy considers it a bug though, as there's no buffer > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page > > buffer that was allocated with __get_free_pages(): > > > > __get_free_pages(GFP_KERNEL, 1) > > > > ... where 1 means an order-1 allocation. > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy > > considers it a bug even though the buffer is 8192 bytes long. Why? > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter > > with ITER_KVEC. > > > > - Eric > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page > boundary when they seem to be fine? This is due to missing the compound page marking, if I remember correctly. However, I tend to leave the pagespan test disabled: it really isn't ready for production use -- there are a lot of missing annotations still. -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-03-21 17:45 ` Kees Cook @ 2019-03-21 17:51 ` Eric Biggers 2019-04-10 3:17 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-03-21 17:51 UTC (permalink / raw) To: Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote: > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote: > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > > > > When running the sha1-asm crypto selftest on arm with > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > > > > > > > usercopy: Kernel memory overwrite attempt detected to spans > > > > multiple pages (offset 0, size 42)! > > > > ------------[ cut here ]------------ > > > > kernel BUG at mm/usercopy.c:102! > > > > Internal error: Oops - BUG: 0 [#1] SMP ARM > > > > Modules linked in: > > > > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted > > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > > > PC is at usercopy_abort+0x68/0x90 > > > > LR is at usercopy_abort+0x68/0x90 > > > > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013 > > > > sp : ea54bc60 ip : 00000010 fp : cccccccd > > > > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009 > > > > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120 > > > > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f > > > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > > > > Control: 30c5387d Table: 40003000 DAC: fffffffd > > > > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval)) > > > > Stack: (0xea54bc60 to 0xea54c000) > > > > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a > > > > 00000000 c0310060 > > > > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8 > > > > ea54cfe0 ea538c00 > > > > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84 > > > > 0000003a c0427a30 > > > > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70 > > > > 0000002a c081cf80 > > > > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8 > > > > c0e0a408 eb074240 > > > > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422 > > > > ebef7480 eb074200 > > > > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014 > > > > c084061c c0428c38 > > > > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40 > > > > 00000002 eabe4e80 > > > > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060 > > > > 00000006 c09d544c > > > > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001 > > > > eb074200 00000000 > > > > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0 > > > > 00000000 c081cf70 > > > > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780 > > > > c0e4ee80 443e9884 > > > > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe > > > > fefefefe fefefefe > > > > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298 > > > > ea4f7a00 c03062bc > > > > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00 > > > > 00000002 eabe4e40 > > > > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314 > > > > eb074200 ea538c00 > > > > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006 > > > > c04dace8 00000006 > > > > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08 > > > > ea4f71a8 c0429420 > > > > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374 > > > > 00000400 ffffffff > > > > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438 > > > > c0e5a2a0 c0e5a2a0 > > > > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013 > > > > c0e5a2a0 c0269470 > > > > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408 > > > > ea537280 0000000e > > > > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4 > > > > c09c9ed0 ea54bf5c > > > > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422 > > > > c09c9ed0 ea537200 > > > > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08 > > > > ea4f71a8 c0426524 > > > > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000 > > > > 00000000 00000000 > > > > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000 > > > > 00000000 00000000 > > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > > 00000000 00000000 > > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > 00000000 00000000 > > > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > > > (__check_object_size+0x2d8/0x448) > > > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > > > (build_test_sglist+0x268/0x2d8) > > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > > > (test_hash_vec_cfg+0x110/0x694) > > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > > > (__alg_test_hash+0x158/0x1b8) > > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > > > Exception stack(0xea54bfb0 to 0xea54bff8) > > > > bfa0: 00000000 00000000 > > > > 00000000 00000000 > > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > > 00000000 00000000 > > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2) > > > > ---[ end trace 190b3cf48e720f78 ]--- > > > > BUG: sleeping function called from invalid context at > > > > include/linux/percpu-rwsem.h:34 > > > > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test > > > > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D > > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > > > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14) > > > > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c) > > > > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158) > > > > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258) > > > > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20) > > > > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344) > > > > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190) > > > > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c) > > > > Exception stack(0xea54bc10 to 0xea54bc58) > > > > bc00: 0000005f 2abb4000 > > > > dd6cd422 dd6cd422 > > > > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04 > > > > 00000000 cccccccd > > > > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff > > > > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90) > > > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > > > (__check_object_size+0x2d8/0x448) > > > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > > > (build_test_sglist+0x268/0x2d8) > > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > > > (test_hash_vec_cfg+0x110/0x694) > > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > > > (__alg_test_hash+0x158/0x1b8) > > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > > > Exception stack(0xea54bfb0 to 0xea54bff8) > > > > bfa0: 00000000 00000000 > > > > 00000000 00000000 > > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > > 00000000 00000000 > > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > > > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't > > > understand why hardened usercopy considers it a bug though, as there's no buffer > > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page > > > buffer that was allocated with __get_free_pages(): > > > > > > __get_free_pages(GFP_KERNEL, 1) > > > > > > ... where 1 means an order-1 allocation. > > > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy > > > considers it a bug even though the buffer is 8192 bytes long. Why? > > > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter > > > with ITER_KVEC. > > > > > > - Eric > > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page > > boundary when they seem to be fine? > > This is due to missing the compound page marking, if I remember > correctly. However, I tend to leave the pagespan test disabled: it > really isn't ready for production use -- there are a lot of missing > annotations still. > So do I need to add __GFP_COMP? Is there any actual reason to do so? Why does hardened usercopy check for it? - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-03-21 17:51 ` Eric Biggers @ 2019-04-10 3:17 ` Eric Biggers 2019-04-10 18:30 ` Kees Cook 0 siblings, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-04-10 3:17 UTC (permalink / raw) To: Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Thu, Mar 21, 2019 at 10:51:22AM -0700, Eric Biggers wrote: > On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote: > > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote: > > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > > > > > When running the sha1-asm crypto selftest on arm with > > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > > > > > > > > > usercopy: Kernel memory overwrite attempt detected to spans > > > > > multiple pages (offset 0, size 42)! > > > > > ------------[ cut here ]------------ > > > > > kernel BUG at mm/usercopy.c:102! > > > > > Internal error: Oops - BUG: 0 [#1] SMP ARM > > > > > Modules linked in: > > > > > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted > > > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > > > > PC is at usercopy_abort+0x68/0x90 > > > > > LR is at usercopy_abort+0x68/0x90 > > > > > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013 > > > > > sp : ea54bc60 ip : 00000010 fp : cccccccd > > > > > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009 > > > > > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120 > > > > > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f > > > > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > > > > > Control: 30c5387d Table: 40003000 DAC: fffffffd > > > > > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval)) > > > > > Stack: (0xea54bc60 to 0xea54c000) > > > > > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a > > > > > 00000000 c0310060 > > > > > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8 > > > > > ea54cfe0 ea538c00 > > > > > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84 > > > > > 0000003a c0427a30 > > > > > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70 > > > > > 0000002a c081cf80 > > > > > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8 > > > > > c0e0a408 eb074240 > > > > > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422 > > > > > ebef7480 eb074200 > > > > > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014 > > > > > c084061c c0428c38 > > > > > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40 > > > > > 00000002 eabe4e80 > > > > > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060 > > > > > 00000006 c09d544c > > > > > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001 > > > > > eb074200 00000000 > > > > > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0 > > > > > 00000000 c081cf70 > > > > > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780 > > > > > c0e4ee80 443e9884 > > > > > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe > > > > > fefefefe fefefefe > > > > > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298 > > > > > ea4f7a00 c03062bc > > > > > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00 > > > > > 00000002 eabe4e40 > > > > > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314 > > > > > eb074200 ea538c00 > > > > > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006 > > > > > c04dace8 00000006 > > > > > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08 > > > > > ea4f71a8 c0429420 > > > > > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374 > > > > > 00000400 ffffffff > > > > > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438 > > > > > c0e5a2a0 c0e5a2a0 > > > > > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013 > > > > > c0e5a2a0 c0269470 > > > > > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408 > > > > > ea537280 0000000e > > > > > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4 > > > > > c09c9ed0 ea54bf5c > > > > > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422 > > > > > c09c9ed0 ea537200 > > > > > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08 > > > > > ea4f71a8 c0426524 > > > > > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000 > > > > > 00000000 00000000 > > > > > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000 > > > > > 00000000 00000000 > > > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > > > 00000000 00000000 > > > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > > 00000000 00000000 > > > > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > > > > (__check_object_size+0x2d8/0x448) > > > > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > > > > (build_test_sglist+0x268/0x2d8) > > > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > > > > (test_hash_vec_cfg+0x110/0x694) > > > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > > > > (__alg_test_hash+0x158/0x1b8) > > > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > > > > Exception stack(0xea54bfb0 to 0xea54bff8) > > > > > bfa0: 00000000 00000000 > > > > > 00000000 00000000 > > > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > > > 00000000 00000000 > > > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2) > > > > > ---[ end trace 190b3cf48e720f78 ]--- > > > > > BUG: sleeping function called from invalid context at > > > > > include/linux/percpu-rwsem.h:34 > > > > > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test > > > > > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D > > > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397 > > > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree) > > > > > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14) > > > > > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c) > > > > > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158) > > > > > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258) > > > > > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20) > > > > > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344) > > > > > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190) > > > > > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c) > > > > > Exception stack(0xea54bc10 to 0xea54bc58) > > > > > bc00: 0000005f 2abb4000 > > > > > dd6cd422 dd6cd422 > > > > > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04 > > > > > 00000000 cccccccd > > > > > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff > > > > > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90) > > > > > [<c030fd60>] (usercopy_abort) from [<c0310060>] > > > > > (__check_object_size+0x2d8/0x448) > > > > > [<c0310060>] (__check_object_size) from [<c0427a30>] > > > > > (build_test_sglist+0x268/0x2d8) > > > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>] > > > > > (test_hash_vec_cfg+0x110/0x694) > > > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>] > > > > > (__alg_test_hash+0x158/0x1b8) > > > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4) > > > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8) > > > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44) > > > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150) > > > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c) > > > > > Exception stack(0xea54bfb0 to 0xea54bff8) > > > > > bfa0: 00000000 00000000 > > > > > 00000000 00000000 > > > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 > > > > > 00000000 00000000 > > > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > > > > > > > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation > > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't > > > > understand why hardened usercopy considers it a bug though, as there's no buffer > > > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page > > > > buffer that was allocated with __get_free_pages(): > > > > > > > > __get_free_pages(GFP_KERNEL, 1) > > > > > > > > ... where 1 means an order-1 allocation. > > > > > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy > > > > considers it a bug even though the buffer is 8192 bytes long. Why? > > > > > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter > > > > with ITER_KVEC. > > > > > > > > - Eric > > > > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page > > > boundary when they seem to be fine? > > > > This is due to missing the compound page marking, if I remember > > correctly. However, I tend to leave the pagespan test disabled: it > > really isn't ready for production use -- there are a lot of missing > > annotations still. > > > > So do I need to add __GFP_COMP? Is there any actual reason to do so? > Why does hardened usercopy check for it? > > - Eric Hi Kees, any answer to this question? - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 3:17 ` Eric Biggers @ 2019-04-10 18:30 ` Kees Cook 2019-04-10 19:07 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-04-10 18:30 UTC (permalink / raw) To: Eric Biggers Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Tue, Apr 9, 2019 at 8:17 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Mar 21, 2019 at 10:51:22AM -0700, Eric Biggers wrote: > > On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote: > > > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote: > > > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > > > > > > When running the sha1-asm crypto selftest on arm with > > > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > > > > > > > > > > > usercopy: Kernel memory overwrite attempt detected to spans > > > > > > multiple pages (offset 0, size 42)! > > > > > > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation > > > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't > > > > > understand why hardened usercopy considers it a bug though, as there's no buffer > > > > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page > > > > > buffer that was allocated with __get_free_pages(): > > > > > > > > > > __get_free_pages(GFP_KERNEL, 1) > > > > > > > > > > ... where 1 means an order-1 allocation. > > > > > > > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy > > > > > considers it a bug even though the buffer is 8192 bytes long. Why? > > > > > > > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter > > > > > with ITER_KVEC. > > > > > > > > > > - Eric > > > > > > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page > > > > boundary when they seem to be fine? > > > > > > This is due to missing the compound page marking, if I remember > > > correctly. However, I tend to leave the pagespan test disabled: it > > > really isn't ready for production use -- there are a lot of missing > > > annotations still. > > > > > > > So do I need to add __GFP_COMP? Is there any actual reason to do so? > > Why does hardened usercopy check for it? > > > > - Eric > > Hi Kees, any answer to this question? Hi! Sorry, this got lost in my inbox. Yes, if you can add __GFP_COMP, that would fix this case. No one has had time lately to track down all these cases, but avoiding adding new ones would be wonderful. :) It's in there because it's a state I'd like to get to in the kernel, but it'll require a lot more work to get there. -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 18:30 ` Kees Cook @ 2019-04-10 19:07 ` Eric Biggers 2019-04-10 21:57 ` Kees Cook 0 siblings, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-04-10 19:07 UTC (permalink / raw) To: Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List On Wed, Apr 10, 2019 at 11:30:57AM -0700, Kees Cook wrote: > On Tue, Apr 9, 2019 at 8:17 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Thu, Mar 21, 2019 at 10:51:22AM -0700, Eric Biggers wrote: > > > On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote: > > > > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote: > > > > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote: > > > > > > > When running the sha1-asm crypto selftest on arm with > > > > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y: > > > > > > > > > > > > > > usercopy: Kernel memory overwrite attempt detected to spans > > > > > > > multiple pages (offset 0, size 42)! > > > > > > > > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation > > > > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't > > > > > > understand why hardened usercopy considers it a bug though, as there's no buffer > > > > > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page > > > > > > buffer that was allocated with __get_free_pages(): > > > > > > > > > > > > __get_free_pages(GFP_KERNEL, 1) > > > > > > > > > > > > ... where 1 means an order-1 allocation. > > > > > > > > > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy > > > > > > considers it a bug even though the buffer is 8192 bytes long. Why? > > > > > > > > > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter > > > > > > with ITER_KVEC. > > > > > > > > > > > > - Eric > > > > > > > > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page > > > > > boundary when they seem to be fine? > > > > > > > > This is due to missing the compound page marking, if I remember > > > > correctly. However, I tend to leave the pagespan test disabled: it > > > > really isn't ready for production use -- there are a lot of missing > > > > annotations still. > > > > > > > > > > So do I need to add __GFP_COMP? Is there any actual reason to do so? > > > Why does hardened usercopy check for it? > > > > > > - Eric > > > > Hi Kees, any answer to this question? > > Hi! Sorry, this got lost in my inbox. Yes, if you can add __GFP_COMP, > that would fix this case. No one has had time lately to track down all > these cases, but avoiding adding new ones would be wonderful. :) > > It's in there because it's a state I'd like to get to in the kernel, > but it'll require a lot more work to get there. > That didn't answer my question. My question is what is the purpose of this? If there was actual buffer overflow when __GFP_COMP isn't specified that would make perfect sense, but AFAICS there isn't. So why does hardened usercopy consider it broken when __GFP_COMP isn't specified? - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 19:07 ` Eric Biggers @ 2019-04-10 21:57 ` Kees Cook 2019-04-10 23:11 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-04-10 21:57 UTC (permalink / raw) To: Eric Biggers Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Wed, Apr 10, 2019 at 12:07 PM Eric Biggers <ebiggers@kernel.org> wrote: > That didn't answer my question. My question is what is the purpose of this? If > there was actual buffer overflow when __GFP_COMP isn't specified that would make > perfect sense, but AFAICS there isn't. So why does hardened usercopy consider > it broken when __GFP_COMP isn't specified? The goal of CONFIG_HARDENED_USERCOPY_PAGESPAN was to detect copies across page boundaries in memory allocated by the page allocator. There appear to be enough cases of allocations that span pages but do not mark them with __GFP_COMP, so this logic hasn't proven useful in the real world (which is why no one should use the ..._PAGESPAN config in production). I'd like to get the kernel to the point where hardened usercopy can correctly do these checks (right now it's mainly only useful at checking for overflows in slub and slab), but it'll take time/focus for a while. No one has had time yet to track all of these down and fix them. (I defer to Laura and Rik on the design of the pagespan checks; they did the bulk of the work there.) Does that help explain it, or am I still missing your question? -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 21:57 ` Kees Cook @ 2019-04-10 23:11 ` Eric Biggers 2019-04-10 23:27 ` Kees Cook 2019-04-11 1:37 ` Rik van Riel 0 siblings, 2 replies; 27+ messages in thread From: Eric Biggers @ 2019-04-10 23:11 UTC (permalink / raw) To: Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Wed, Apr 10, 2019 at 02:57:46PM -0700, Kees Cook wrote: > On Wed, Apr 10, 2019 at 12:07 PM Eric Biggers <ebiggers@kernel.org> wrote: > > That didn't answer my question. My question is what is the purpose of this? If > > there was actual buffer overflow when __GFP_COMP isn't specified that would make > > perfect sense, but AFAICS there isn't. So why does hardened usercopy consider > > it broken when __GFP_COMP isn't specified? > > The goal of CONFIG_HARDENED_USERCOPY_PAGESPAN was to detect copies > across page boundaries in memory allocated by the page allocator. > There appear to be enough cases of allocations that span pages but do > not mark them with __GFP_COMP, so this logic hasn't proven useful in > the real world (which is why no one should use the ..._PAGESPAN config > in production). I'd like to get the kernel to the point where hardened > usercopy can correctly do these checks (right now it's mainly only > useful at checking for overflows in slub and slab), but it'll take > time/focus for a while. No one has had time yet to track all of these > down and fix them. (I defer to Laura and Rik on the design of the > pagespan checks; they did the bulk of the work there.) > > Does that help explain it, or am I still missing your question? > > -- > Kees Cook You've explained *what* it does again, but not *why*. *Why* do you want hardened usercopy to detect copies across page boundaries, when there is no actual buffer overflow? - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 23:11 ` Eric Biggers @ 2019-04-10 23:27 ` Kees Cook 2019-04-11 17:58 ` Eric Biggers 2019-04-11 1:37 ` Rik van Riel 1 sibling, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-04-10 23:27 UTC (permalink / raw) To: Eric Biggers Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote: > You've explained *what* it does again, but not *why*. *Why* do you want > hardened usercopy to detect copies across page boundaries, when there is no > actual buffer overflow? But that *is* how it determines it was a buffer overflow: "if you cross page boundaries (of a non-compound allocation), it *is* a buffer overflow". This assertion, however, is flawed because many contiguous allocations are not marked as being grouped together when it reality they were. It was an attempt to get allocation size information out of the page allocator, similar to how slab can be queries about allocation size. I'm open to improvements here, since it's obviously broken in its current state. :) -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 23:27 ` Kees Cook @ 2019-04-11 17:58 ` Eric Biggers 2019-04-11 18:33 ` Kees Cook 0 siblings, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-04-11 17:58 UTC (permalink / raw) To: Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Wed, Apr 10, 2019 at 04:27:28PM -0700, Kees Cook wrote: > On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote: > > You've explained *what* it does again, but not *why*. *Why* do you want > > hardened usercopy to detect copies across page boundaries, when there is no > > actual buffer overflow? > > But that *is* how it determines it was a buffer overflow: "if you > cross page boundaries (of a non-compound allocation), it *is* a buffer > overflow". This assertion, however, is flawed because many contiguous > allocations are not marked as being grouped together when it reality > they were. It was an attempt to get allocation size information out of > the page allocator, similar to how slab can be queries about > allocation size. I'm open to improvements here, since it's obviously > broken in its current state. :) > > -- > Kees Cook Well, I'm still at a loss as to whether I'm actually supposed to "fix" this by adding __GFP_COMP, or whether you're saying the option is broken anyway so I shouldn't bother doing anything. IIUC, even the kernel stack is still not marked __GFP_COMP, so copies to/from the stack can trigger this too, despite this being reported over 2 years ago (http://lkml.iu.edu/hypermail/linux/kernel/1701.2/01450.html)? CONFIG_HARDENED_USERCOPY_PAGESPAN is even disabled in syzbot because you already said the option is broken and should not be used. I worry that people will enable all the hardened usercopy options "because security", then when the pagespan check breaks something they will disable all hardened usercopy options, because they don't understand the individual options. Providing broken options is actively harmful, IMO. - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-11 17:58 ` Eric Biggers @ 2019-04-11 18:33 ` Kees Cook 2019-04-11 19:26 ` Eric Biggers 0 siblings, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-04-11 18:33 UTC (permalink / raw) To: Eric Biggers, Dmitry Vyukov Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Thu, Apr 11, 2019 at 10:58 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, Apr 10, 2019 at 04:27:28PM -0700, Kees Cook wrote: > > On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > You've explained *what* it does again, but not *why*. *Why* do you want > > > hardened usercopy to detect copies across page boundaries, when there is no > > > actual buffer overflow? > > > > But that *is* how it determines it was a buffer overflow: "if you > > cross page boundaries (of a non-compound allocation), it *is* a buffer > > overflow". This assertion, however, is flawed because many contiguous > > allocations are not marked as being grouped together when it reality > > they were. It was an attempt to get allocation size information out of > > the page allocator, similar to how slab can be queries about > > allocation size. I'm open to improvements here, since it's obviously > > broken in its current state. :) > > > > -- > > Kees Cook > > Well, I'm still at a loss as to whether I'm actually supposed to "fix" this by > adding __GFP_COMP, or whether you're saying the option is broken anyway so I I would love it if you could fix it, yes. > shouldn't bother doing anything. IIUC, even the kernel stack is still not > marked __GFP_COMP, so copies to/from the stack can trigger this too, despite > this being reported over 2 years ago > (http://lkml.iu.edu/hypermail/linux/kernel/1701.2/01450.html)? > CONFIG_HARDENED_USERCOPY_PAGESPAN is even disabled in syzbot because you already > said the option is broken and should not be used. stacks are checked before PAGESPAN, so that particular problem should no longer be present since commit 7bff3c069973 ("mm/usercopy.c: no check page span for stack objects"). > I worry that people will enable all the hardened usercopy options "because > security", then when the pagespan check breaks something they will disable all > hardened usercopy options, because they don't understand the individual options. > Providing broken options is actively harmful, IMO. It's behind EXPERT, default-n, and says: When a multi-page allocation is done without __GFP_COMP, hardened usercopy will reject attempts to copy it. There are, however, several cases of this in the kernel that have not all been removed. This config is intended to be used only while trying to find such users. I'd rather leave it since it's still useful. Perhaps it could be switched to WARN by default and we reenable it in syzbot to improve its utility there? diff --git a/mm/usercopy.c b/mm/usercopy.c index 14faadcedd06..6e7e28fe062b 100644 --- a/mm/usercopy.c +++ b/mm/usercopy.c @@ -208,8 +208,13 @@ static inline void check_page_span(const void *ptr, unsigned long n, */ is_reserved = PageReserved(page); is_cma = is_migrate_cma_page(page); - if (!is_reserved && !is_cma) - usercopy_abort("spans multiple pages", NULL, to_user, 0, n); + if (!is_reserved && !is_cma) { + usercopy_warn("spans multiple pages without __GFP_COMP", + NULL, to_user, + (unsigned long)ptr & (unsigned long)PAGE_MASK, + n); + return; + } for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) { page = virt_to_head_page(ptr); -- Kees Cook ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-11 18:33 ` Kees Cook @ 2019-04-11 19:26 ` Eric Biggers 2019-04-11 19:28 ` [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP Eric Biggers 2019-04-11 20:36 ` crypto: Kernel memory overwrite attempt detected to spans multiple pages Kees Cook 0 siblings, 2 replies; 27+ messages in thread From: Eric Biggers @ 2019-04-11 19:26 UTC (permalink / raw) To: Kees Cook Cc: Dmitry Vyukov, Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Thu, Apr 11, 2019 at 11:33:04AM -0700, Kees Cook wrote: > On Thu, Apr 11, 2019 at 10:58 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Wed, Apr 10, 2019 at 04:27:28PM -0700, Kees Cook wrote: > > > On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > You've explained *what* it does again, but not *why*. *Why* do you want > > > > hardened usercopy to detect copies across page boundaries, when there is no > > > > actual buffer overflow? > > > > > > But that *is* how it determines it was a buffer overflow: "if you > > > cross page boundaries (of a non-compound allocation), it *is* a buffer > > > overflow". This assertion, however, is flawed because many contiguous > > > allocations are not marked as being grouped together when it reality > > > they were. It was an attempt to get allocation size information out of > > > the page allocator, similar to how slab can be queries about > > > allocation size. I'm open to improvements here, since it's obviously > > > broken in its current state. :) > > > > > > -- > > > Kees Cook > > > > Well, I'm still at a loss as to whether I'm actually supposed to "fix" this by > > adding __GFP_COMP, or whether you're saying the option is broken anyway so I > > I would love it if you could fix it, yes. > > > shouldn't bother doing anything. IIUC, even the kernel stack is still not > > marked __GFP_COMP, so copies to/from the stack can trigger this too, despite > > this being reported over 2 years ago > > (http://lkml.iu.edu/hypermail/linux/kernel/1701.2/01450.html)? > > CONFIG_HARDENED_USERCOPY_PAGESPAN is even disabled in syzbot because you already > > said the option is broken and should not be used. > > stacks are checked before PAGESPAN, so that particular problem should > no longer be present since commit 7bff3c069973 ("mm/usercopy.c: no > check page span for stack objects"). > > > I worry that people will enable all the hardened usercopy options "because > > security", then when the pagespan check breaks something they will disable all > > hardened usercopy options, because they don't understand the individual options. > > Providing broken options is actively harmful, IMO. > > It's behind EXPERT, default-n, and says: > > When a multi-page allocation is done without __GFP_COMP, > hardened usercopy will reject attempts to copy it. There are, > however, several cases of this in the kernel that have not all > been removed. This config is intended to be used only while > trying to find such users. > > I'd rather leave it since it's still useful. > > Perhaps it could be switched to WARN by default and we reenable it in > syzbot to improve its utility there? > > diff --git a/mm/usercopy.c b/mm/usercopy.c > index 14faadcedd06..6e7e28fe062b 100644 > --- a/mm/usercopy.c > +++ b/mm/usercopy.c > @@ -208,8 +208,13 @@ static inline void check_page_span(const void > *ptr, unsigned long n, > */ > is_reserved = PageReserved(page); > is_cma = is_migrate_cma_page(page); > - if (!is_reserved && !is_cma) > - usercopy_abort("spans multiple pages", NULL, to_user, 0, n); > + if (!is_reserved && !is_cma) { > + usercopy_warn("spans multiple pages without __GFP_COMP", > + NULL, to_user, > + (unsigned long)ptr & (unsigned long)PAGE_MASK, > + n); > + return; > + } > > for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) { > page = virt_to_head_page(ptr); > > > -- > Kees Cook Well, I guess I'll just add __GFP_COMP so I at least don't get spammed with useless bug reports. But I don't think it's in any way acceptable to change the semantics of the kernel's page allocator but only under some obscure config option, don't document it anywhere, ignore the known problems for years, say that the option is broken anyway so it shouldn't be used, and have to exchange 15 emails to get anything resembling an explanation. - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-11 19:26 ` Eric Biggers @ 2019-04-11 19:28 ` Eric Biggers 2019-04-11 20:32 ` Kees Cook 2019-04-11 20:36 ` crypto: Kernel memory overwrite attempt detected to spans multiple pages Kees Cook 1 sibling, 1 reply; 27+ messages in thread From: Eric Biggers @ 2019-04-11 19:28 UTC (permalink / raw) To: linux-crypto, Herbert Xu Cc: Kees Cook, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott, Rik van Riel From: Eric Biggers <ebiggers@google.com> This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't incorrectly report a buffer overflow when the destination of copy_from_iter() spans the page boundary in the 2-page buffer. Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions") Signed-off-by: Eric Biggers <ebiggers@google.com> --- crypto/testmgr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crypto/testmgr.c b/crypto/testmgr.c index 0f6bfb6ce6a46..3522c0bed2492 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order) int i; for (i = 0; i < XBUFSIZE; i++) { - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order); + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP, + order); if (!buf[i]) goto err_free_buf; } -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-11 19:28 ` [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP Eric Biggers @ 2019-04-11 20:32 ` Kees Cook 2019-04-12 5:38 ` Dmitry Vyukov 2019-04-15 2:24 ` Matthew Wilcox 0 siblings, 2 replies; 27+ messages in thread From: Kees Cook @ 2019-04-11 20:32 UTC (permalink / raw) To: Eric Biggers, Rik van Riel, Matthew Wilcox Cc: linux-crypto, Herbert Xu, Kees Cook, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott On Thu, Apr 11, 2019 at 12:31 PM Eric Biggers <ebiggers@kernel.org> wrote: > > From: Eric Biggers <ebiggers@google.com> > > This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't > incorrectly report a buffer overflow when the destination of > copy_from_iter() spans the page boundary in the 2-page buffer. > > Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions") > Signed-off-by: Eric Biggers <ebiggers@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> > --- > crypto/testmgr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > index 0f6bfb6ce6a46..3522c0bed2492 100644 > --- a/crypto/testmgr.c > +++ b/crypto/testmgr.c > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order) > int i; > > for (i = 0; i < XBUFSIZE; i++) { > - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order); > + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP, > + order); Is there a reason __GFP_COMP isn't automatically included in all page allocations? (Or rather, it seems like the exception is when things should NOT be considered part of the same allocation, so something like __GFP_SINGLE should exist?.) -Kees > if (!buf[i]) > goto err_free_buf; > } > -- > 2.21.0.392.gf8f6787159e-goog > -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-11 20:32 ` Kees Cook @ 2019-04-12 5:38 ` Dmitry Vyukov 2019-04-15 2:24 ` Matthew Wilcox 1 sibling, 0 replies; 27+ messages in thread From: Dmitry Vyukov @ 2019-04-12 5:38 UTC (permalink / raw) To: Kees Cook Cc: Eric Biggers, Rik van Riel, Matthew Wilcox, linux-crypto, Herbert Xu, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott On Thu, Apr 11, 2019 at 10:32 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Apr 11, 2019 at 12:31 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't > > incorrectly report a buffer overflow when the destination of > > copy_from_iter() spans the page boundary in the 2-page buffer. > > > > Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions") > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > --- > > crypto/testmgr.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > > index 0f6bfb6ce6a46..3522c0bed2492 100644 > > --- a/crypto/testmgr.c > > +++ b/crypto/testmgr.c > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order) > > int i; > > > > for (i = 0; i < XBUFSIZE; i++) { > > - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order); > > + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP, > > + order); > > Is there a reason __GFP_COMP isn't automatically included in all page > allocations? (Or rather, it seems like the exception is when things > should NOT be considered part of the same allocation, so something > like __GFP_SINGLE should exist?.) It would be reasonable if __get_free_pages would automatically mark consecutive pages as consecutive. When these should not be considered part of the same allocation? Is it possible to free them separately? Will that BUG with __GFP_COMP mark? I understand that there can be a weak "these are actually the same allocation, but I would like to think about them as separate". But potentially we can ignore such cases. > -Kees > > > if (!buf[i]) > > goto err_free_buf; > > } > > -- > > 2.21.0.392.gf8f6787159e-goog > > > > > -- > Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-11 20:32 ` Kees Cook 2019-04-12 5:38 ` Dmitry Vyukov @ 2019-04-15 2:24 ` Matthew Wilcox 2019-04-15 2:46 ` Herbert Xu 1 sibling, 1 reply; 27+ messages in thread From: Matthew Wilcox @ 2019-04-15 2:24 UTC (permalink / raw) To: Kees Cook Cc: Eric Biggers, Rik van Riel, linux-crypto, Herbert Xu, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott, linux-mm On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote: > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order) > > int i; > > > > for (i = 0; i < XBUFSIZE; i++) { > > - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order); > > + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP, > > + order); > > Is there a reason __GFP_COMP isn't automatically included in all page > allocations? (Or rather, it seems like the exception is when things > should NOT be considered part of the same allocation, so something > like __GFP_SINGLE should exist?.) The question is not whether or not things should be considered part of the same allocation. The question is whether the allocation is of a compound page or of N consecutive pages. Now you're asking what the difference is, and it's whether you need to be able to be able to call compound_head(), compound_order(), PageTail() or use a compound_dtor. If you don't, then you can save some time at allocation & free by not specifying __GFP_COMP. I'll agree this is not documented well, and maybe most multi-page allocations do want __GFP_COMP and we should invert that bit, but __GFP_SINGLE doesn't seem like the right antonym to __GFP_COMP to me. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-15 2:24 ` Matthew Wilcox @ 2019-04-15 2:46 ` Herbert Xu 2019-04-16 2:18 ` Matthew Wilcox 0 siblings, 1 reply; 27+ messages in thread From: Herbert Xu @ 2019-04-15 2:46 UTC (permalink / raw) To: Matthew Wilcox Cc: Kees Cook, Eric Biggers, Rik van Riel, linux-crypto, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott, linux-mm On Sun, Apr 14, 2019 at 07:24:12PM -0700, Matthew Wilcox wrote: > On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote: > > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order) > > > int i; > > > > > > for (i = 0; i < XBUFSIZE; i++) { > > > - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order); > > > + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP, > > > + order); > > > > Is there a reason __GFP_COMP isn't automatically included in all page > > allocations? (Or rather, it seems like the exception is when things > > should NOT be considered part of the same allocation, so something > > like __GFP_SINGLE should exist?.) > > The question is not whether or not things should be considered part of the > same allocation. The question is whether the allocation is of a compound > page or of N consecutive pages. Now you're asking what the difference is, > and it's whether you need to be able to be able to call compound_head(), > compound_order(), PageTail() or use a compound_dtor. If you don't, then > you can save some time at allocation & free by not specifying __GFP_COMP. Thanks for clarifying Matthew. Eric, this means that we should not use __GFP_COMP here just to silent what is clearly a broken warning. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-15 2:46 ` Herbert Xu @ 2019-04-16 2:18 ` Matthew Wilcox 2019-04-16 3:14 ` Kees Cook 0 siblings, 1 reply; 27+ messages in thread From: Matthew Wilcox @ 2019-04-16 2:18 UTC (permalink / raw) To: Herbert Xu Cc: Kees Cook, Eric Biggers, Rik van Riel, linux-crypto, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott, linux-mm On Mon, Apr 15, 2019 at 10:46:15AM +0800, Herbert Xu wrote: > On Sun, Apr 14, 2019 at 07:24:12PM -0700, Matthew Wilcox wrote: > > On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote: > > > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order) > > > > int i; > > > > > > > > for (i = 0; i < XBUFSIZE; i++) { > > > > - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order); > > > > + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP, > > > > + order); > > > > > > Is there a reason __GFP_COMP isn't automatically included in all page > > > allocations? (Or rather, it seems like the exception is when things > > > should NOT be considered part of the same allocation, so something > > > like __GFP_SINGLE should exist?.) > > > > The question is not whether or not things should be considered part of the > > same allocation. The question is whether the allocation is of a compound > > page or of N consecutive pages. Now you're asking what the difference is, > > and it's whether you need to be able to be able to call compound_head(), > > compound_order(), PageTail() or use a compound_dtor. If you don't, then > > you can save some time at allocation & free by not specifying __GFP_COMP. > > Thanks for clarifying Matthew. > > Eric, this means that we should not use __GFP_COMP here just to > silent what is clearly a broken warning. I agree; if the crypto code is never going to try to go from the address of a byte in the allocation back to the head page, then there's no need to specify GFP_COMP. But that leaves us in the awkward situation where HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether 'ptr + n - 1' lies within the same allocation as ptr. Without using a compound page, there's no indication in the VM structures that these two pages were allocated as part of the same allocation. We could force all multi-page allocations to be compound pages if HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break something. We could make it catch fewer problems by succeeding if the page is not compound. I don't know, these all seem like bad choices to me. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-16 2:18 ` Matthew Wilcox @ 2019-04-16 3:14 ` Kees Cook 2019-04-17 4:08 ` Matthew Wilcox 0 siblings, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-04-16 3:14 UTC (permalink / raw) To: Matthew Wilcox Cc: Herbert Xu, Kees Cook, Eric Biggers, Rik van Riel, linux-crypto, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott, Linux-MM On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote: > I agree; if the crypto code is never going to try to go from the address of > a byte in the allocation back to the head page, then there's no need to > specify GFP_COMP. > > But that leaves us in the awkward situation where > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether > 'ptr + n - 1' lies within the same allocation as ptr. Without using > a compound page, there's no indication in the VM structures that these > two pages were allocated as part of the same allocation. > > We could force all multi-page allocations to be compound pages if > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break > something. We could make it catch fewer problems by succeeding if the > page is not compound. I don't know, these all seem like bad choices > to me. If GFP_COMP is _not_ the correct signal about adjacent pages being part of the same allocation, then I agree: we need to drop this check entirely from PAGESPAN. Is there anything else that indicates this property? (Or where might we be able to store that info?) There are other pagespan checks, though, so those could stay. But I'd really love to gain page allocator allocation size checking ... -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-16 3:14 ` Kees Cook @ 2019-04-17 4:08 ` Matthew Wilcox 2019-04-17 8:09 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 27+ messages in thread From: Matthew Wilcox @ 2019-04-17 4:08 UTC (permalink / raw) To: Kees Cook Cc: Herbert Xu, Eric Biggers, Rik van Riel, linux-crypto, Dmitry Vyukov, Geert Uytterhoeven, linux-security-module, Linux ARM, Linux Kernel Mailing List, Laura Abbott, Linux-MM On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote: > On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote: > > I agree; if the crypto code is never going to try to go from the address of > > a byte in the allocation back to the head page, then there's no need to > > specify GFP_COMP. > > > > But that leaves us in the awkward situation where > > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether > > 'ptr + n - 1' lies within the same allocation as ptr. Without using > > a compound page, there's no indication in the VM structures that these > > two pages were allocated as part of the same allocation. > > > > We could force all multi-page allocations to be compound pages if > > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break > > something. We could make it catch fewer problems by succeeding if the > > page is not compound. I don't know, these all seem like bad choices > > to me. > > If GFP_COMP is _not_ the correct signal about adjacent pages being > part of the same allocation, then I agree: we need to drop this check > entirely from PAGESPAN. Is there anything else that indicates this > property? (Or where might we be able to store that info?) As far as I know, the page allocator does not store size information anywhere, unless you use GFP_COMP. That's why you have to pass the 'order' to free_pages() and __free_pages(). It's also why alloc_pages_exact() works (follow all the way into split_page()). > There are other pagespan checks, though, so those could stay. But I'd > really love to gain page allocator allocation size checking ... I think that's a great idea, but I'm not sure how you'll be able to do that. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-17 4:08 ` Matthew Wilcox @ 2019-04-17 8:09 ` Russell King - ARM Linux admin 2019-04-17 9:54 ` Robin Murphy 0 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux admin @ 2019-04-17 8:09 UTC (permalink / raw) To: Matthew Wilcox Cc: Kees Cook, Linux ARM, Herbert Xu, Rik van Riel, Linux Kernel Mailing List, Eric Biggers, Linux-MM, linux-security-module, Geert Uytterhoeven, linux-crypto, Laura Abbott, Dmitry Vyukov On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote: > On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote: > > On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote: > > > I agree; if the crypto code is never going to try to go from the address of > > > a byte in the allocation back to the head page, then there's no need to > > > specify GFP_COMP. > > > > > > But that leaves us in the awkward situation where > > > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether > > > 'ptr + n - 1' lies within the same allocation as ptr. Without using > > > a compound page, there's no indication in the VM structures that these > > > two pages were allocated as part of the same allocation. > > > > > > We could force all multi-page allocations to be compound pages if > > > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break > > > something. We could make it catch fewer problems by succeeding if the > > > page is not compound. I don't know, these all seem like bad choices > > > to me. > > > > If GFP_COMP is _not_ the correct signal about adjacent pages being > > part of the same allocation, then I agree: we need to drop this check > > entirely from PAGESPAN. Is there anything else that indicates this > > property? (Or where might we be able to store that info?) > > As far as I know, the page allocator does not store size information > anywhere, unless you use GFP_COMP. That's why you have to pass > the 'order' to free_pages() and __free_pages(). It's also why > alloc_pages_exact() works (follow all the way into split_page()). > > > There are other pagespan checks, though, so those could stay. But I'd > > really love to gain page allocator allocation size checking ... > > I think that's a great idea, but I'm not sure how you'll be able to > do that. However, we have had code (maybe historically now) that has allocated a higher order page and then handed back pages that it doesn't need - for example, when the code requires multiple contiguous pages but does not require a power-of-2 size of contiguous pages. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP 2019-04-17 8:09 ` Russell King - ARM Linux admin @ 2019-04-17 9:54 ` Robin Murphy 0 siblings, 0 replies; 27+ messages in thread From: Robin Murphy @ 2019-04-17 9:54 UTC (permalink / raw) To: Russell King - ARM Linux admin, Matthew Wilcox Cc: Kees Cook, Rik van Riel, Linux Kernel Mailing List, Eric Biggers, Linux-MM, linux-security-module, Geert Uytterhoeven, linux-crypto, Dmitry Vyukov, Laura Abbott, Linux ARM, Herbert Xu On 17/04/2019 09:09, Russell King - ARM Linux admin wrote: > On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote: >> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote: >>> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote: >>>> I agree; if the crypto code is never going to try to go from the address of >>>> a byte in the allocation back to the head page, then there's no need to >>>> specify GFP_COMP. >>>> >>>> But that leaves us in the awkward situation where >>>> HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether >>>> 'ptr + n - 1' lies within the same allocation as ptr. Without using >>>> a compound page, there's no indication in the VM structures that these >>>> two pages were allocated as part of the same allocation. >>>> >>>> We could force all multi-page allocations to be compound pages if >>>> HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break >>>> something. We could make it catch fewer problems by succeeding if the >>>> page is not compound. I don't know, these all seem like bad choices >>>> to me. >>> >>> If GFP_COMP is _not_ the correct signal about adjacent pages being >>> part of the same allocation, then I agree: we need to drop this check >>> entirely from PAGESPAN. Is there anything else that indicates this >>> property? (Or where might we be able to store that info?) >> >> As far as I know, the page allocator does not store size information >> anywhere, unless you use GFP_COMP. That's why you have to pass >> the 'order' to free_pages() and __free_pages(). It's also why >> alloc_pages_exact() works (follow all the way into split_page()). >> >>> There are other pagespan checks, though, so those could stay. But I'd >>> really love to gain page allocator allocation size checking ... >> >> I think that's a great idea, but I'm not sure how you'll be able to >> do that. > > However, we have had code (maybe historically now) that has allocated > a higher order page and then handed back pages that it doesn't need - > for example, when the code requires multiple contiguous pages but does > not require a power-of-2 size of contiguous pages. 'git grep alloc_pages_exact' suggests it's not historical yet... Robin. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-11 19:26 ` Eric Biggers 2019-04-11 19:28 ` [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP Eric Biggers @ 2019-04-11 20:36 ` Kees Cook 2019-04-11 20:56 ` Eric Biggers 1 sibling, 1 reply; 27+ messages in thread From: Kees Cook @ 2019-04-11 20:36 UTC (permalink / raw) To: Eric Biggers Cc: Dmitry Vyukov, Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Thu, Apr 11, 2019 at 12:26 PM Eric Biggers <ebiggers@kernel.org> wrote: > Well, I guess I'll just add __GFP_COMP so I at least don't get spammed with > useless bug reports. Thanks, I appreciate it. > But I don't think it's in any way acceptable to change the semantics of the > kernel's page allocator but only under some obscure config option, don't > document it anywhere, ignore the known problems for years, say that the option > is broken anyway so it shouldn't be used, and have to exchange 15 emails to get > anything resembling an explanation. I understand what you mean, yeah. I'm sorry I wasn't clear about it earlier. What do you think might help the situation as far as documentation? -- Kees Cook ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-11 20:36 ` crypto: Kernel memory overwrite attempt detected to spans multiple pages Kees Cook @ 2019-04-11 20:56 ` Eric Biggers 0 siblings, 0 replies; 27+ messages in thread From: Eric Biggers @ 2019-04-11 20:56 UTC (permalink / raw) To: Kees Cook Cc: Dmitry Vyukov, Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott, Rik van Riel On Thu, Apr 11, 2019 at 01:36:37PM -0700, Kees Cook wrote: > On Thu, Apr 11, 2019 at 12:26 PM Eric Biggers <ebiggers@kernel.org> wrote: > > Well, I guess I'll just add __GFP_COMP so I at least don't get spammed with > > useless bug reports. > > Thanks, I appreciate it. > > > But I don't think it's in any way acceptable to change the semantics of the > > kernel's page allocator but only under some obscure config option, don't > > document it anywhere, ignore the known problems for years, say that the option > > is broken anyway so it shouldn't be used, and have to exchange 15 emails to get > > anything resembling an explanation. > > I understand what you mean, yeah. I'm sorry I wasn't clear about it > earlier. What do you think might help the situation as far as > documentation? > Explanation in Documentation/core-api/memory-allocation.rst of when to use __GFP_COMP and why. Where "why" includes the real underlying reason why it's designed the way it is, not just "you now need to provide this flag in order to stop the hardened usercopy thing from crashing, even though previously it meant something else, because that's the way it is." Also a brief, improved explanation of __GFP_COMP in include/linux/gfp.h. Also get Documentation/security/self-protection.rst up to date with what's actually in the kernel. Currently it doesn't mention hardened usercopy at all, and it's unclear about what's supported now vs. what is future work. And actually fix the known problems with the pagespan check, not ignore them for years. If not feasible, remove the option. - Eric ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages 2019-04-10 23:11 ` Eric Biggers 2019-04-10 23:27 ` Kees Cook @ 2019-04-11 1:37 ` Rik van Riel 1 sibling, 0 replies; 27+ messages in thread From: Rik van Riel @ 2019-04-11 1:37 UTC (permalink / raw) To: Eric Biggers, Kees Cook Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM, Linux Crypto Mailing List, Linux Kernel Mailing List, Laura Abbott [-- Attachment #1: Type: text/plain, Size: 563 bytes --] On Wed, 2019-04-10 at 16:11 -0700, Eric Biggers wrote: > You've explained *what* it does again, but not *why*. *Why* do you > want > hardened usercopy to detect copies across page boundaries, when there > is no > actual buffer overflow? When some subsystem in the kernel allocates multiple pages without _GFP_COMP, there is no way afterwards to detect exactly how many pages it allocated. In other words, there is no way to see how large the buffer is, nor whether the copy operation in question would overflow it. -- All Rights Reversed. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-04-17 9:54 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-19 11:54 crypto: Kernel memory overwrite attempt detected to spans multiple pages Geert Uytterhoeven 2019-03-19 17:09 ` Eric Biggers 2019-03-20 18:57 ` Eric Biggers 2019-03-21 17:45 ` Kees Cook 2019-03-21 17:51 ` Eric Biggers 2019-04-10 3:17 ` Eric Biggers 2019-04-10 18:30 ` Kees Cook 2019-04-10 19:07 ` Eric Biggers 2019-04-10 21:57 ` Kees Cook 2019-04-10 23:11 ` Eric Biggers 2019-04-10 23:27 ` Kees Cook 2019-04-11 17:58 ` Eric Biggers 2019-04-11 18:33 ` Kees Cook 2019-04-11 19:26 ` Eric Biggers 2019-04-11 19:28 ` [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP Eric Biggers 2019-04-11 20:32 ` Kees Cook 2019-04-12 5:38 ` Dmitry Vyukov 2019-04-15 2:24 ` Matthew Wilcox 2019-04-15 2:46 ` Herbert Xu 2019-04-16 2:18 ` Matthew Wilcox 2019-04-16 3:14 ` Kees Cook 2019-04-17 4:08 ` Matthew Wilcox 2019-04-17 8:09 ` Russell King - ARM Linux admin 2019-04-17 9:54 ` Robin Murphy 2019-04-11 20:36 ` crypto: Kernel memory overwrite attempt detected to spans multiple pages Kees Cook 2019-04-11 20:56 ` Eric Biggers 2019-04-11 1:37 ` Rik van Riel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).