* sg_io HARDENED_USERCOPY_PAGESPAN trace @ 2016-12-28 21:40 Dave Jones 2016-12-29 7:56 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2016-12-28 21:40 UTC (permalink / raw) To: Kees Cook; +Cc: Linux Kernel One of my machines won't boot 4.10rc1, because it hit this: usercopy: kernel memory overwrite attempt detected to ffff88042601cff8 (<spans multiple pages>) (12 bytes) ------------[ cut here ]------------ kernel BUG at mm/usercopy.c:75! invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC CPU: 5 PID: 483 Comm: ata_id Not tainted 4.10.0-rc1-backup-debug+ task: ffff880427bd2ec0 task.stack: ffffc900011d0000 RIP: 0010:__check_object_size+0xf4/0x316 RSP: 0018:ffffc900011d3b28 EFLAGS: 00010282 RAX: 000000000000006a RBX: ffff88042601cff8 RCX: 0000000000000000 RDX: 0000000000000000 RSI: ffff88043dd4cc28 RDI: ffff88043dd4cc28 RBP: ffffc900011d3b60 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000001 R11: 0000000000000001 R12: 000000000000000c R13: ffffea0010980700 R14: 0000000000000000 R15: ffff88042601d004 FS: 00007f05c3011b40(0000) GS:ffff88043dd40000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f05c270ca30 CR3: 0000000427b38000 CR4: 00000000001406e0 Call Trace: sg_io+0x113/0x470 ? __might_fault+0x43/0xa0 ? __check_object_size+0x11b/0x316 scsi_cmd_ioctl+0x335/0x4d0 ? __might_sleep+0x4b/0x90 scsi_cmd_blk_ioctl+0x42/0x50 sd_ioctl+0x85/0x110 blkdev_ioctl+0x53b/0xa20 block_ioctl+0x3d/0x50 do_vfs_ioctl+0xa3/0x730 ? __context_tracking_exit.part.6+0x4a/0x190 ? __seccomp_filter+0x67/0x220 SyS_ioctl+0x41/0x70 do_syscall_64+0x7b/0x700 entry_SYSCALL64_slow_path+0x25/0x25 RIP: 0033:0x7f05c271bcc7 RSP: 002b:00007ffe1559d8e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f05c271bcc7 RDX: 00007ffe1559d930 RSI: 0000000000002285 RDI: 0000000000000003 RBP: 000056259fdfd010 R08: 0000000000000000 R09: 0000000000000000 R10: 00007f05c3011b40 R11: 0000000000000246 R12: 00007ffe1559ef1c R13: 00007ffe1559db90 R14: 0000000000000003 R15: 0000000000000000 Code: f9 01 00 00 49 c7 c0 3d 86 ef 8e 48 c7 c2 50 63 f0 8e 48 c7 c6 ed f4 ee 8e 4d 89 e1 48 89 d9 48 c7 c7 b0 58 ef 8e e8 fc f3 f9 ff <0f> 0b 4c 89 ea 4c 89 e6 48 89 df e8 3c b4 ff ff 48 85 c0 49 89 RIP: __check_object_size+0xf4/0x316 RSP: ffffc900011d3b28 usercopy: kernel memory overwrite attempt detected to ffff88042624cff8 (<spans multiple pages>) (16 bytes) For now I've disabled HARDENED_USERCOPY_PAGESPAN. I suspect this only showed up on the one machine because it's got a RAID6 array, and I don't use md anywhere else. Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2016-12-28 21:40 sg_io HARDENED_USERCOPY_PAGESPAN trace Dave Jones @ 2016-12-29 7:56 ` Christoph Hellwig 2016-12-29 15:43 ` Dave Jones 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2016-12-29 7:56 UTC (permalink / raw) To: Dave Jones, Kees Cook, Linux Kernel On Wed, Dec 28, 2016 at 04:40:16PM -0500, Dave Jones wrote: > sg_io+0x113/0x470 Can you resolve that to a source line using a gdb? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2016-12-29 7:56 ` Christoph Hellwig @ 2016-12-29 15:43 ` Dave Jones 2016-12-30 13:37 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2016-12-29 15:43 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Kees Cook, Linux Kernel On Wed, Dec 28, 2016 at 11:56:42PM -0800, Christoph Hellwig wrote: > On Wed, Dec 28, 2016 at 04:40:16PM -0500, Dave Jones wrote: > > sg_io+0x113/0x470 > > Can you resolve that to a source line using a gdb? It's the copy_from_user in an inlined copy of blk_fill_sghdr_rq. Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2016-12-29 15:43 ` Dave Jones @ 2016-12-30 13:37 ` Christoph Hellwig 2016-12-30 15:01 ` Dave Jones 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2016-12-30 13:37 UTC (permalink / raw) To: Dave Jones, Christoph Hellwig, Kees Cook, Linux Kernel On Thu, Dec 29, 2016 at 10:43:51AM -0500, Dave Jones wrote: > On Wed, Dec 28, 2016 at 11:56:42PM -0800, Christoph Hellwig wrote: > > On Wed, Dec 28, 2016 at 04:40:16PM -0500, Dave Jones wrote: > > > sg_io+0x113/0x470 > > > > Can you resolve that to a source line using a gdb? > > It's the copy_from_user in an inlined copy of blk_fill_sghdr_rq. That must be this line right at the beginning of blk_fill_sghdr_rq if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len)) return -EFAULT; We're copying the SCSI CDB from the userspace pointer inside the hdr we copied earlier into the request. req->cmd is set to req->__cmd which is a u8 array with 16 members in struct request by default, but if hdr->cmd_len is bigger than BLK_MAX_CDB (16) we do a separate allocation for it in the caller: if (hdr->cmd_len > BLK_MAX_CDB) { rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL); if (!rq->cmd) goto out_put_request; } so I'm not really sure what the problem here could be. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2016-12-30 13:37 ` Christoph Hellwig @ 2016-12-30 15:01 ` Dave Jones 2016-12-30 15:10 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2016-12-30 15:01 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Kees Cook, Linux Kernel On Fri, Dec 30, 2016 at 05:37:12AM -0800, Christoph Hellwig wrote: > On Thu, Dec 29, 2016 at 10:43:51AM -0500, Dave Jones wrote: > > On Wed, Dec 28, 2016 at 11:56:42PM -0800, Christoph Hellwig wrote: > > > On Wed, Dec 28, 2016 at 04:40:16PM -0500, Dave Jones wrote: > > > > sg_io+0x113/0x470 > > > > > > Can you resolve that to a source line using a gdb? > > > > It's the copy_from_user in an inlined copy of blk_fill_sghdr_rq. > > That must be this line right at the beginning of blk_fill_sghdr_rq > > if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len)) > return -EFAULT; > > We're copying the SCSI CDB from the userspace pointer inside the hdr > we copied earlier into the request. > > req->cmd is set to req->__cmd which is a u8 array with 16 members in > struct request by default, but if hdr->cmd_len is bigger than BLK_MAX_CDB > (16) we do a separate allocation for it in the caller: > > if (hdr->cmd_len > BLK_MAX_CDB) { > rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL); > if (!rq->cmd) > goto out_put_request; > } > > so I'm not really sure what the problem here could be. I threw this debug printk into the pagespan code to see what exactly it was complaining about.. ptr:ffff88042614cff8 end:ffff88042614d003 n:c so it was copying 12 bytes that spanned two pages. >From my reading of the config option help text, this thing is complaining that wasn't allocated with __GFP_COMP maybe ? Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2016-12-30 15:01 ` Dave Jones @ 2016-12-30 15:10 ` Christoph Hellwig 2017-01-03 21:48 ` Kees Cook 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2016-12-30 15:10 UTC (permalink / raw) To: Dave Jones, Christoph Hellwig, Kees Cook, Linux Kernel On Fri, Dec 30, 2016 at 10:01:39AM -0500, Dave Jones wrote: > I threw this debug printk into the pagespan code to see what exactly > it was complaining about.. > > ptr:ffff88042614cff8 end:ffff88042614d003 n:c > > so it was copying 12 bytes that spanned two pages. > >From my reading of the config option help text, this thing is > complaining that wasn't allocated with __GFP_COMP maybe ? If this is on a devie using blk-mq the block core will use high order allocations (as high as possible) to allocate the requests for each queue, so struct request could very well span multiple pages. But I don't see what __GFP_COMP would have to do with user copy annoations. As all requests for a queue are freed togeth again there is no point in setting __GFP_COMP for the request allocations. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2016-12-30 15:10 ` Christoph Hellwig @ 2017-01-03 21:48 ` Kees Cook 2017-01-08 9:44 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Kees Cook @ 2017-01-03 21:48 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Dave Jones, Linux Kernel On Fri, Dec 30, 2016 at 7:10 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Fri, Dec 30, 2016 at 10:01:39AM -0500, Dave Jones wrote: >> I threw this debug printk into the pagespan code to see what exactly >> it was complaining about.. >> >> ptr:ffff88042614cff8 end:ffff88042614d003 n:c >> >> so it was copying 12 bytes that spanned two pages. >> >From my reading of the config option help text, this thing is >> complaining that wasn't allocated with __GFP_COMP maybe ? There are a lot of cases of "missing" __GFP_COMP, which is why HARDENED_USERCOPY_PAGESPAN defaults to "n". > If this is on a devie using blk-mq the block core will use high > order allocations (as high as possible) to allocate the requests > for each queue, so struct request could very well span multiple > pages. But I don't see what __GFP_COMP would have to do with > user copy annoations. As all requests for a queue are freed > togeth again there is no point in setting __GFP_COMP for the > request allocations. Does it hurt anything to mark these pages as allocated "together" via __GFP_COMP? -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sg_io HARDENED_USERCOPY_PAGESPAN trace 2017-01-03 21:48 ` Kees Cook @ 2017-01-08 9:44 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2017-01-08 9:44 UTC (permalink / raw) To: Kees Cook; +Cc: Christoph Hellwig, Dave Jones, Linux Kernel On Tue, Jan 03, 2017 at 01:48:03PM -0800, Kees Cook wrote: > There are a lot of cases of "missing" __GFP_COMP, which is why > HARDENED_USERCOPY_PAGESPAN defaults to "n". > > > If this is on a devie using blk-mq the block core will use high > > order allocations (as high as possible) to allocate the requests > > for each queue, so struct request could very well span multiple > > pages. But I don't see what __GFP_COMP would have to do with > > user copy annoations. As all requests for a queue are freed > > togeth again there is no point in setting __GFP_COMP for the > > request allocations. > > Does it hurt anything to mark these pages as allocated "together" via > __GFP_COMP? It don't think it would hurt the block code - it only allocates the pages once, and frees them once. But I think hijacking your feature on top of a totally unrelated flag is a horrible idea. __GFP_COMP is about refcounting the allocation, not about anything else. The prime use case of high order allocations is to use them as a single memory object, which might include user copies. So as-is I think HARDENED_USERCOPY_PAGESPAN is a misfeature, it needs to be opt-in for allocations where we might not copy over the span of pages, not opt-out. And I suspect there aren't going to be all that many opt-out candidates. > > -Kees > > -- > Kees Cook > Nexus Security ---end quoted text--- ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-08 9:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-28 21:40 sg_io HARDENED_USERCOPY_PAGESPAN trace Dave Jones 2016-12-29 7:56 ` Christoph Hellwig 2016-12-29 15:43 ` Dave Jones 2016-12-30 13:37 ` Christoph Hellwig 2016-12-30 15:01 ` Dave Jones 2016-12-30 15:10 ` Christoph Hellwig 2017-01-03 21:48 ` Kees Cook 2017-01-08 9:44 ` Christoph Hellwig
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).