[PATCH] block: fix NULL pointer dereference in account statistics with IDE

[PATCH] block: fix NULL pointer dereference in account statistics with IDE

[Logan Gunthorpe]

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(-)

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;
 
-- 
2.20.1

Re: [PATCH] block: fix NULL pointer dereference in account statistics with IDE

[Jens Axboe]

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)

-- 
Jens Axboe

Re: [PATCH] block: fix NULL pointer dereference in account statistics with IDE

[Logan Gunthorpe]

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

Re: [PATCH] block: fix NULL pointer dereference in account statistics with IDE

[Jens Axboe]

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.

-- 
Jens Axboe