From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: References: <10360653.ov98egbaqx@natalenko.name> <2864697.7uzmEJovl2@natalenko.name> From: Kees Cook Date: Tue, 17 Apr 2018 13:25:49 -0700 Message-ID: Subject: Re: usercopy whitelist woe in scsi_sense_cache To: Oleksandr Natalenko , Jens Axboe , Bart Van Assche , Paolo Valente Cc: David Windsor , "James E.J. Bottomley" , "Martin K. Petersen" , linux-scsi@vger.kernel.org, LKML , Christoph Hellwig , Hannes Reinecke , Johannes Thumshirn , linux-block@vger.kernel.org Content-Type: text/plain; charset="UTF-8" List-ID: On Tue, Apr 17, 2018 at 1:20 PM, Kees Cook wrote: > On Tue, Apr 17, 2018 at 1:03 PM, Kees Cook wrote: >> The above bfq_dispatch_request+0x99/0xad0 is still >> __bfq_dispatch_request at block/bfq-iosched.c:3902, just with KASAN >> removed. 0x99 is 153 decimal: >> >> (gdb) disass bfq_dispatch_request >> Dump of assembler code for function bfq_dispatch_request: >> ... >> 0xffffffff8134b2ad <+141>: test %rax,%rax >> 0xffffffff8134b2b0 <+144>: je 0xffffffff8134b2bd >> >> 0xffffffff8134b2b2 <+146>: addl $0x1,0x100(%rax) >> 0xffffffff8134b2b9 <+153>: addl $0x1,0x3c(%rbx) >> 0xffffffff8134b2bd <+157>: orl $0x2,0x18(%r12) >> 0xffffffff8134b2c3 <+163>: test %ebp,%ebp >> 0xffffffff8134b2c5 <+165>: je 0xffffffff8134b2ce >> >> 0xffffffff8134b2c7 <+167>: mov 0x108(%r14),%rax >> 0xffffffff8134b2ce <+174>: mov %r15,%rdi >> 0xffffffff8134b2d1 <+177>: callq 0xffffffff81706f90 <_raw_spin_unlock_irq> >> >> Just as a sanity-check, at +157 %r12 should be rq, rq_flags is 0x18 >> offset from, $0x2 is RQF_STARTED, so that maps to "rq->rq_flags |= >> RQF_STARTED", the next C statement. I don't know what +146 is, though? >> An increment of something 256 bytes offset? There's a lot of inline >> fun and reordering happening here, so I'm ignoring that for the >> moment. > > No -- I'm reading this wrong. The RIP is the IP _after_ the trap, so > +146 is the offender. > > [ 29.284746] watchpoint @ ffff95d41a0fe580 triggered > [ 29.285349] sense before:ffff95d41f45f700 after:ffff95d41f45f701 (@ffff95d41a > 0fe580) > [ 29.286176] elevator before:ffff95d419419c00 after:ffff95d419419c00 > [ 29.286847] elevator_data before:ffff95d419418c00 after:ffff95d419418c00 > ... > [ 29.295069] RIP: 0010:bfq_dispatch_request+0x99/0xbb0 > [ 29.295622] RSP: 0018:ffffb26e01707a40 EFLAGS: 00000002 > [ 29.296181] RAX: ffff95d41a0fe480 RBX: ffff95d419418c00 RCX: ffff95d419418c08 > > RAX is ffff95d41a0fe480 and sense is stored at ffff95d41a0fe580, > exactly 0x100 away. > > WTF is this addl? What are the chances? :P Two ++ statements in a row separate by a collapsed goto. FML. :) ... bfqq->dispatched++; goto inc_in_driver_start_rq; ... inc_in_driver_start_rq: bfqd->rq_in_driver++; ... And there's the 0x100 (256): struct bfq_queue { ... int dispatched; /* 256 4 */ So bfqq is corrupted somewhere... I'll keep digging. I hope you're all enjoying my live debugging transcript. ;) -Kees -- Kees Cook Pixel Security