| Message ID | 20191210184704.24081-1-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix NULL pointer dereference in account statistics with IDE | expand |
On 12/10/19 11:47 AM, Logan Gunthorpe wrote: > The IDE driver creates some passthru requests which never get > submitted to the block layer in such a way that blk_account_io_start() > gets called. However, the driver still calls __blk_mq_end_request() in > ide_end_rq() which will call blk_account_io_completion() which tries > to dereferences req->part which is never set. See ide_prep_sense() for > an example of where these requests come from. > > To fix this, blk_account_io_completion() and blk_account_io_done() > should do nothing if req->part is not set. > > The back trace of this bug is: > > BUG: kernel NULL pointer dereference, address: 000002ac > #PF: supervisor write access in kernel mode > #PF: error_code(0x0002) - not-present page > *pde = 00000000 > Oops: 0002 [#1] > CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted > 5.4.0-rc2-00011-g48d9b0d43105e #1 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 > 04/01/2014 > Workqueue: kblockd drive_rq_insert_work > EIP: blk_account_io_completion+0x7a/0xf0 > Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee > 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4 > 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b > EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000 > ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54 > DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046 > CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0 > Call Trace: > <IRQ> > blk_update_request+0x85/0x420 > ide_end_rq+0x38/0xa0 > ide_complete_rq+0x3d/0x70 > cdrom_newpc_intr+0x258/0xba0 > ide_intr+0x135/0x250 > __handle_irq_event_percpu+0x3e/0x250 > handle_irq_event_percpu+0x1f/0x50 > handle_irq_event+0x32/0x60 > handle_level_irq+0x6c/0x110 > handle_irq+0x72/0xa0 > </IRQ> > do_IRQ+0x45/0xad > common_interrupt+0x115/0x11c Why not just: diff --git a/block/blk.h b/block/blk.h index 6842f28c033e..d7407b5d0200 100644 --- a/block/blk.h +++ b/block/blk.h @@ -250,7 +250,7 @@ int blk_dev_init(void); */ static inline bool blk_do_io_stat(struct request *rq) { - return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT); + return rq->part && rq->rq_disk && (rq->rq_flags & RQF_IO_STAT); } static inline void req_set_nomerge(struct request_queue *q, struct request *req)
On 2019-12-10 11:59 a.m., Jens Axboe wrote: > On 12/10/19 11:47 AM, Logan Gunthorpe wrote: >> The IDE driver creates some passthru requests which never get >> submitted to the block layer in such a way that blk_account_io_start() >> gets called. However, the driver still calls __blk_mq_end_request() in >> ide_end_rq() which will call blk_account_io_completion() which tries >> to dereferences req->part which is never set. See ide_prep_sense() for >> an example of where these requests come from. >> >> To fix this, blk_account_io_completion() and blk_account_io_done() >> should do nothing if req->part is not set. >> >> The back trace of this bug is: >> >> BUG: kernel NULL pointer dereference, address: 000002ac >> #PF: supervisor write access in kernel mode >> #PF: error_code(0x0002) - not-present page >> *pde = 00000000 >> Oops: 0002 [#1] >> CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted >> 5.4.0-rc2-00011-g48d9b0d43105e #1 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 >> 04/01/2014 >> Workqueue: kblockd drive_rq_insert_work >> EIP: blk_account_io_completion+0x7a/0xf0 >> Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee >> 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4 >> 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b >> EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000 >> ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54 >> DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046 >> CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0 >> Call Trace: >> <IRQ> >> blk_update_request+0x85/0x420 >> ide_end_rq+0x38/0xa0 >> ide_complete_rq+0x3d/0x70 >> cdrom_newpc_intr+0x258/0xba0 >> ide_intr+0x135/0x250 >> __handle_irq_event_percpu+0x3e/0x250 >> handle_irq_event_percpu+0x1f/0x50 >> handle_irq_event+0x32/0x60 >> handle_level_irq+0x6c/0x110 >> handle_irq+0x72/0xa0 >> </IRQ> >> do_IRQ+0x45/0xad >> common_interrupt+0x115/0x11c > > Why not just: > > diff --git a/block/blk.h b/block/blk.h > index 6842f28c033e..d7407b5d0200 100644 > --- a/block/blk.h > +++ b/block/blk.h > @@ -250,7 +250,7 @@ int blk_dev_init(void); > */ > static inline bool blk_do_io_stat(struct request *rq) > { > - return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT); > + return rq->part && rq->rq_disk && (rq->rq_flags & RQF_IO_STAT); > } Because blk_account_io_start() also checks blk_do_io_stat() and, in that case, rq->part will never be set (seeing that's the function that typically sets it); thus that solution would disable stats entirely. Logan
On 12/10/19 12:02 PM, Logan Gunthorpe wrote: > > > On 2019-12-10 11:59 a.m., Jens Axboe wrote: >> On 12/10/19 11:47 AM, Logan Gunthorpe wrote: >>> The IDE driver creates some passthru requests which never get >>> submitted to the block layer in such a way that blk_account_io_start() >>> gets called. However, the driver still calls __blk_mq_end_request() in >>> ide_end_rq() which will call blk_account_io_completion() which tries >>> to dereferences req->part which is never set. See ide_prep_sense() for >>> an example of where these requests come from. >>> >>> To fix this, blk_account_io_completion() and blk_account_io_done() >>> should do nothing if req->part is not set. >>> >>> The back trace of this bug is: >>> >>> BUG: kernel NULL pointer dereference, address: 000002ac >>> #PF: supervisor write access in kernel mode >>> #PF: error_code(0x0002) - not-present page >>> *pde = 00000000 >>> Oops: 0002 [#1] >>> CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted >>> 5.4.0-rc2-00011-g48d9b0d43105e #1 >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 >>> 04/01/2014 >>> Workqueue: kblockd drive_rq_insert_work >>> EIP: blk_account_io_completion+0x7a/0xf0 >>> Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee >>> 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4 >>> 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b >>> EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000 >>> ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54 >>> DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046 >>> CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0 >>> Call Trace: >>> <IRQ> >>> blk_update_request+0x85/0x420 >>> ide_end_rq+0x38/0xa0 >>> ide_complete_rq+0x3d/0x70 >>> cdrom_newpc_intr+0x258/0xba0 >>> ide_intr+0x135/0x250 >>> __handle_irq_event_percpu+0x3e/0x250 >>> handle_irq_event_percpu+0x1f/0x50 >>> handle_irq_event+0x32/0x60 >>> handle_level_irq+0x6c/0x110 >>> handle_irq+0x72/0xa0 >>> </IRQ> >>> do_IRQ+0x45/0xad >>> common_interrupt+0x115/0x11c >> >> Why not just: >> >> diff --git a/block/blk.h b/block/blk.h >> index 6842f28c033e..d7407b5d0200 100644 >> --- a/block/blk.h >> +++ b/block/blk.h >> @@ -250,7 +250,7 @@ int blk_dev_init(void); >> */ >> static inline bool blk_do_io_stat(struct request *rq) >> { >> - return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT); >> + return rq->part && rq->rq_disk && (rq->rq_flags & RQF_IO_STAT); >> } > > Because blk_account_io_start() also checks blk_do_io_stat() and, in that > case, rq->part will never be set (seeing that's the function that > typically sets it); thus that solution would disable stats entirely. Gotcha. I'm fine with the patch you posted in that case.
diff --git a/block/blk-core.c b/block/blk-core.c index a1e228752083..68c309ce6735 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1310,7 +1310,7 @@ EXPORT_SYMBOL_GPL(blk_rq_err_bytes); void blk_account_io_completion(struct request *req, unsigned int bytes) { - if (blk_do_io_stat(req)) { + if (req->part && blk_do_io_stat(req)) { const int sgrp = op_stat_group(req_op(req)); struct hd_struct *part; @@ -1328,7 +1328,8 @@ void blk_account_io_done(struct request *req, u64 now) * normal IO on queueing nor completion. Accounting the * containing request is enough. */ - if (blk_do_io_stat(req) && !(req->rq_flags & RQF_FLUSH_SEQ)) { + if (req->part && blk_do_io_stat(req) && + !(req->rq_flags & RQF_FLUSH_SEQ)) { const int sgrp = op_stat_group(req_op(req)); struct hd_struct *part;
The IDE driver creates some passthru requests which never get submitted to the block layer in such a way that blk_account_io_start() gets called. However, the driver still calls __blk_mq_end_request() in ide_end_rq() which will call blk_account_io_completion() which tries to dereferences req->part which is never set. See ide_prep_sense() for an example of where these requests come from. To fix this, blk_account_io_completion() and blk_account_io_done() should do nothing if req->part is not set. The back trace of this bug is: BUG: kernel NULL pointer dereference, address: 000002ac #PF: supervisor write access in kernel mode #PF: error_code(0x0002) - not-present page *pde = 00000000 Oops: 0002 [#1] CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted 5.4.0-rc2-00011-g48d9b0d43105e #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 Workqueue: kblockd drive_rq_insert_work EIP: blk_account_io_completion+0x7a/0xf0 Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000 ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54 DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046 CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0 Call Trace: <IRQ> blk_update_request+0x85/0x420 ide_end_rq+0x38/0xa0 ide_complete_rq+0x3d/0x70 cdrom_newpc_intr+0x258/0xba0 ide_intr+0x135/0x250 __handle_irq_event_percpu+0x3e/0x250 handle_irq_event_percpu+0x1f/0x50 handle_irq_event+0x32/0x60 handle_level_irq+0x6c/0x110 handle_irq+0x72/0xa0 </IRQ> do_IRQ+0x45/0xad common_interrupt+0x115/0x11c Fixes: 48d9b0d43105 ("block: account statistics for passthrough requests") Reported-by: kernel test robot <rong.a.chen@intel.com> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- block/blk-core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)