[PATCH] nvme: fix NULL pointer dereference

[PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

blk_mq_tag_to_rq can return NULL and this condition must be checked

[  147.861050] BUG: KASAN: null-ptr-deref in nvme_irq+0xfc/0x410
[  147.861326] Write of size 2 at addr 0000000000000122 by task kworker/u4:1/56
[  147.861675]
[  147.861756] CPU: 1 PID: 56 Comm: kworker/u4:1 Not tainted 5.9.0-rc4+ #83
[  147.862657] Workqueue: nvme-wq nvme_scan_work
[  147.862876] Call Trace:
[  147.863002]  <IRQ>
[  147.863112]  dump_stack+0x7d/0xb0
[  147.863280]  kasan_report.cold+0x6a/0x7e
[  147.863478]  ? nvme_irq+0xfc/0x410
[  147.863650]  nvme_irq+0xfc/0x410
[  147.863815]  ? __x64_sys_getrandom+0xb0/0xb0
[  147.864030]  ? nvme_del_cq_end+0x70/0x70
[  147.864229]  __handle_irq_event_percpu+0x69/0x280
[  147.864465]  handle_irq_event_percpu+0x6a/0xe0
[  147.864689]  ? __handle_irq_event_percpu+0x280/0x280
[  147.864939]  ? _raw_spin_lock+0x75/0xd0
[  147.865131]  ? _raw_read_lock_irq+0x30/0x30
[  147.865345]  handle_irq_event+0x57/0x86
[  147.865537]  handle_edge_irq+0xe5/0x2d0
[  147.865732]  asm_call_on_stack+0x12/0x20
[  147.865929]  </IRQ>
[  147.866038]  common_interrupt+0xad/0x110
[  147.866236]  asm_common_interrupt+0x1e/0x40
[  147.866447] RIP: 0010:__asan_load4+0x40/0xa0
[  147.866663] Code: 00 00 ff 48 39 f8 77 56 48 8d 47 03 48 89 c2 83 e2 07 48 83 fa 02 76 2f 48 b9 00 00 00 00 00 fc ff df 48 c1 e8 03 0f b6 04 08 <84>0
[  147.867577] RSP: 0000:ffff8880666e6d48 EFLAGS: 00000a06
[  147.867837] RAX: 0000000000000000 RBX: ffff8880666e6e28 RCX: dffffc0000000000
[  147.868190] RDX: 0000000000000003 RSI: ffffffffb053e44f RDI: ffff8880666e6e30
[  147.868544] RBP: 0000000000000013 R08: ffffffffafb5ee6f R09: fffffbfff64977ad
[  147.868897] R10: ffffffffb24bbd63 R11: fffffbfff64977ac R12: ffffffffb053e44f
[  147.869250] R13: 0000000000000000 R14: ffff8880666d8040 R15: 0000000000000130

Signed-off-by: Tong Zhang <ztong0001@gmail.com>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 899d2f4d7ab6..725d2263a0f5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 	}
 
 	req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
+	if (!req)
+		return;
 	trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
 	if (!nvme_try_complete_req(req, cqe->status, cqe->result))
 		nvme_pci_complete_rq(req);
-- 
2.25.1

[PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

blk_mq_tag_to_rq can return NULL and this condition must be checked

[  147.861050] BUG: KASAN: null-ptr-deref in nvme_irq+0xfc/0x410
[  147.861326] Write of size 2 at addr 0000000000000122 by task kworker/u4:1/56
[  147.861675]
[  147.861756] CPU: 1 PID: 56 Comm: kworker/u4:1 Not tainted 5.9.0-rc4+ #83
[  147.862657] Workqueue: nvme-wq nvme_scan_work
[  147.862876] Call Trace:
[  147.863002]  <IRQ>
[  147.863112]  dump_stack+0x7d/0xb0
[  147.863280]  kasan_report.cold+0x6a/0x7e
[  147.863478]  ? nvme_irq+0xfc/0x410
[  147.863650]  nvme_irq+0xfc/0x410
[  147.863815]  ? __x64_sys_getrandom+0xb0/0xb0
[  147.864030]  ? nvme_del_cq_end+0x70/0x70
[  147.864229]  __handle_irq_event_percpu+0x69/0x280
[  147.864465]  handle_irq_event_percpu+0x6a/0xe0
[  147.864689]  ? __handle_irq_event_percpu+0x280/0x280
[  147.864939]  ? _raw_spin_lock+0x75/0xd0
[  147.865131]  ? _raw_read_lock_irq+0x30/0x30
[  147.865345]  handle_irq_event+0x57/0x86
[  147.865537]  handle_edge_irq+0xe5/0x2d0
[  147.865732]  asm_call_on_stack+0x12/0x20
[  147.865929]  </IRQ>
[  147.866038]  common_interrupt+0xad/0x110
[  147.866236]  asm_common_interrupt+0x1e/0x40
[  147.866447] RIP: 0010:__asan_load4+0x40/0xa0
[  147.866663] Code: 00 00 ff 48 39 f8 77 56 48 8d 47 03 48 89 c2 83 e2 07 48 83 fa 02 76 2f 48 b9 00 00 00 00 00 fc ff df 48 c1 e8 03 0f b6 04 08 <84>0
[  147.867577] RSP: 0000:ffff8880666e6d48 EFLAGS: 00000a06
[  147.867837] RAX: 0000000000000000 RBX: ffff8880666e6e28 RCX: dffffc0000000000
[  147.868190] RDX: 0000000000000003 RSI: ffffffffb053e44f RDI: ffff8880666e6e30
[  147.868544] RBP: 0000000000000013 R08: ffffffffafb5ee6f R09: fffffbfff64977ad
[  147.868897] R10: ffffffffb24bbd63 R11: fffffbfff64977ac R12: ffffffffb053e44f
[  147.869250] R13: 0000000000000000 R14: ffff8880666d8040 R15: 0000000000000130

Signed-off-by: Tong Zhang <ztong0001@gmail.com>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 899d2f4d7ab6..725d2263a0f5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 	}
 
 	req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
+	if (!req)
+		return;
 	trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
 	if (!nvme_try_complete_req(req, cqe->status, cqe->result))
 		nvme_pci_complete_rq(req);
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix NULL pointer dereference

[Keith Busch]

On Wed, Sep 16, 2020 at 11:36:49AM -0400, Tong Zhang wrote:
> @@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
>  	}
>  
>  	req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
> +	if (!req)
> +		return;

As I mentioned before, blk_mq_tag_to_rq() returns NULL if the tag
exceeds the depth. We already verify the tag prior to calling this
function, so what's the real root cause for how we're winding up with
NULL here? I'm only asking this because it sounds like there's a bug
somewhere else and this change is masking over it.


>  	trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
>  	if (!nvme_try_complete_req(req, cqe->status, cqe->result))
>  		nvme_pci_complete_rq(req);

Re: [PATCH] nvme: fix NULL pointer dereference

[Keith Busch]

On Wed, Sep 16, 2020 at 11:36:49AM -0400, Tong Zhang wrote:
> @@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
>  	}
>  
>  	req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
> +	if (!req)
> +		return;

As I mentioned before, blk_mq_tag_to_rq() returns NULL if the tag
exceeds the depth. We already verify the tag prior to calling this
function, so what's the real root cause for how we're winding up with
NULL here? I'm only asking this because it sounds like there's a bug
somewhere else and this change is masking over it.


>  	trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
>  	if (!nvme_try_complete_req(req, cqe->status, cqe->result))
>  		nvme_pci_complete_rq(req);

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

The command_id in CQE is writable by NVMe controller, driver should
check its sanity before using it.
- Tong

On Wed, Sep 16, 2020 at 12:54 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Wed, Sep 16, 2020 at 11:36:49AM -0400, Tong Zhang wrote:
> > @@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
> >       }
> >
> >       req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
> > +     if (!req)
> > +             return;
>
> As I mentioned before, blk_mq_tag_to_rq() returns NULL if the tag
> exceeds the depth. We already verify the tag prior to calling this
> function, so what's the real root cause for how we're winding up with
> NULL here? I'm only asking this because it sounds like there's a bug
> somewhere else and this change is masking over it.
>
>
> >       trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
> >       if (!nvme_try_complete_req(req, cqe->status, cqe->result))
> >               nvme_pci_complete_rq(req);

Re: [PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

The command_id in CQE is writable by NVMe controller, driver should
check its sanity before using it.
- Tong

On Wed, Sep 16, 2020 at 12:54 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Wed, Sep 16, 2020 at 11:36:49AM -0400, Tong Zhang wrote:
> > @@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
> >       }
> >
> >       req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
> > +     if (!req)
> > +             return;
>
> As I mentioned before, blk_mq_tag_to_rq() returns NULL if the tag
> exceeds the depth. We already verify the tag prior to calling this
> function, so what's the real root cause for how we're winding up with
> NULL here? I'm only asking this because it sounds like there's a bug
> somewhere else and this change is masking over it.
>
>
> >       trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
> >       if (!nvme_try_complete_req(req, cqe->status, cqe->result))
> >               nvme_pci_complete_rq(req);

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix NULL pointer dereference

[Keith Busch]

On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote:
> The command_id in CQE is writable by NVMe controller, driver should
> check its sanity before using it.

We already do that.

Re: [PATCH] nvme: fix NULL pointer dereference

[Keith Busch]

On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote:
> The command_id in CQE is writable by NVMe controller, driver should
> check its sanity before using it.

We already do that.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

Hmm..Yeah.. I see your point.
I was naivly thinking the command_id was the culprit.

On Thu, Sep 17, 2020 at 1:14 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote:
> > The command_id in CQE is writable by NVMe controller, driver should
> > check its sanity before using it.
>
> We already do that.

Re: [PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

Hmm..Yeah.. I see your point.
I was naivly thinking the command_id was the culprit.

On Thu, Sep 17, 2020 at 1:14 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote:
> > The command_id in CQE is writable by NVMe controller, driver should
> > check its sanity before using it.
>
> We already do that.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

Please correct me if I am wrong.
After a bit more digging I found out that it is indeed command_id got
corrupted is causing this problem. Although the tag and command_id
range is checked like you said, the elements in rqs cannot be
guaranteed to be not NULL. thus although the range check is passed,
blk_mq_tag_to_rq() can still return NULL. It is clear that the current
sanitization is not enough and there's more implication about this --
when all rqs got populated, a corrupted command_id may silently
corrupt other data not belonging to the current command.

- Tong

On Thu, Sep 17, 2020 at 8:44 PM Tong Zhang <ztong0001@gmail.com> wrote:
>
> Hmm..Yeah.. I see your point.
> I was naivly thinking the command_id was the culprit.
>
> On Thu, Sep 17, 2020 at 1:14 PM Keith Busch <kbusch@kernel.org> wrote:
> >
> > On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote:
> > > The command_id in CQE is writable by NVMe controller, driver should
> > > check its sanity before using it.
> >
> > We already do that.

Re: [PATCH] nvme: fix NULL pointer dereference

[Tong Zhang]

Please correct me if I am wrong.
After a bit more digging I found out that it is indeed command_id got
corrupted is causing this problem. Although the tag and command_id
range is checked like you said, the elements in rqs cannot be
guaranteed to be not NULL. thus although the range check is passed,
blk_mq_tag_to_rq() can still return NULL. It is clear that the current
sanitization is not enough and there's more implication about this --
when all rqs got populated, a corrupted command_id may silently
corrupt other data not belonging to the current command.

- Tong

On Thu, Sep 17, 2020 at 8:44 PM Tong Zhang <ztong0001@gmail.com> wrote:
>
> Hmm..Yeah.. I see your point.
> I was naivly thinking the command_id was the culprit.
>
> On Thu, Sep 17, 2020 at 1:14 PM Keith Busch <kbusch@kernel.org> wrote:
> >
> > On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote:
> > > The command_id in CQE is writable by NVMe controller, driver should
> > > check its sanity before using it.
> >
> > We already do that.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix NULL pointer dereference

[Keith Busch]

On Thu, Sep 17, 2020 at 11:32:12PM -0400, Tong Zhang wrote:
> Please correct me if I am wrong.
> After a bit more digging I found out that it is indeed command_id got
> corrupted is causing this problem. Although the tag and command_id
> range is checked like you said, the elements in rqs cannot be
> guaranteed to be not NULL. thus although the range check is passed,
> blk_mq_tag_to_rq() can still return NULL. 

I think your describing a sequence problem in initialization. We
shouldn't have interrupts wired up to uninitialized tagsets.

A more appropriate sequence would setup request_irq() after the tagset
is ready. It makes handling a failed irq setup a bit weird for io
queues, though.

> It is clear that the current sanitization is not enough and there's
> more implication about this -- when all rqs got populated, a corrupted
> command_id may silently corrupt other data not belonging to the
> current command.

The block layer doesn't do anything with requests that haven't been
started, so if your controller completes non-existent commands, then
nothing particular will happen with the rqs.

If the request had been started and the controller provides a corrupted
completion, then the fault lies entirely with the controller and you
should raise that issue with your vendor. There's no way the driver can
distinguish a genuine completion from a corrupted one.

Re: [PATCH] nvme: fix NULL pointer dereference

[Keith Busch]

On Thu, Sep 17, 2020 at 11:32:12PM -0400, Tong Zhang wrote:
> Please correct me if I am wrong.
> After a bit more digging I found out that it is indeed command_id got
> corrupted is causing this problem. Although the tag and command_id
> range is checked like you said, the elements in rqs cannot be
> guaranteed to be not NULL. thus although the range check is passed,
> blk_mq_tag_to_rq() can still return NULL. 

I think your describing a sequence problem in initialization. We
shouldn't have interrupts wired up to uninitialized tagsets.

A more appropriate sequence would setup request_irq() after the tagset
is ready. It makes handling a failed irq setup a bit weird for io
queues, though.

> It is clear that the current sanitization is not enough and there's
> more implication about this -- when all rqs got populated, a corrupted
> command_id may silently corrupt other data not belonging to the
> current command.

The block layer doesn't do anything with requests that haven't been
started, so if your controller completes non-existent commands, then
nothing particular will happen with the rqs.

If the request had been started and the controller provides a corrupted
completion, then the fault lies entirely with the controller and you
should raise that issue with your vendor. There's no way the driver can
distinguish a genuine completion from a corrupted one.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme