| Message ID | 20220831014730.17566-1-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/srp: Set scmnd->result only when scmnd is not NULL | expand |
On 31/08/2022 09:47, yangx.jy@fujitsu.com wrote: > This change fixes the following kernel NULL pointer dereference > which is reproduced by blktests srp/007 occasionally. > > BUG: kernel NULL pointer dereference, address: 0000000000000170 > #PF: supervisor write access in kernel mode > #PF: error_code(0x0002) - not-present page > PGD 0 P4D 0 > Oops: 0002 [#1] PREEMPT SMP NOPTI > CPU: 0 PID: 9 Comm: kworker/0:1H Kdump: loaded Not tainted 6.0.0-rc1+ #37 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-29-g6a62e0cb0dfe-prebuilt.qemu.org 04/01/2014 > Workqueue: 0x0 (kblockd) > RIP: 0010:srp_recv_done+0x176/0x500 [ib_srp] > Code: 00 4d 85 ff 0f 84 52 02 00 00 48 c7 82 80 02 00 00 00 00 00 00 4c 89 df 4c 89 14 24 e8 53 d3 4a f6 4c 8b 14 24 41 0f b6 42 13 <41> 89 87 70 01 00 00 41 0f b6 52 12 f6 c2 02 74 44 41 8b 42 1c b9 > RSP: 0018:ffffaef7c0003e28 EFLAGS: 00000282 > RAX: 0000000000000000 RBX: ffff9bc9486dea60 RCX: 0000000000000000 > RDX: 0000000000000102 RSI: ffffffffb76bbd0e RDI: 00000000ffffffff > RBP: ffff9bc980099a00 R08: 0000000000000001 R09: 0000000000000001 > R10: ffff9bca53ef0000 R11: ffff9bc980099a10 R12: ffff9bc956e14000 > R13: ffff9bc9836b9cb0 R14: ffff9bc9557b4480 R15: 0000000000000000 > FS: 0000000000000000(0000) GS:ffff9bc97ec00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000000000170 CR3: 0000000007e04000 CR4: 00000000000006f0 > Call Trace: > <IRQ> > __ib_process_cq+0xb7/0x280 [ib_core] > ib_poll_handler+0x2b/0x130 [ib_core] > irq_poll_softirq+0x93/0x150 > __do_softirq+0xee/0x4b8 > irq_exit_rcu+0xf7/0x130 > sysvec_apic_timer_interrupt+0x8e/0xc0 > </IRQ> > > Fixes: aef9ec39c47f ("IB: Add SCSI RDMA Protocol (SRP) initiator") > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > index 7720ea270ed8..528cdd0daba4 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) > if (scmnd) { > req = scsi_cmd_priv(scmnd); > scmnd = srp_claim_req(ch, req, NULL, scmnd); > + scmnd->result = rsp->status; > } else { > shost_printk(KERN_ERR, target->scsi_host, > "Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n", > @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) > > return; > } > - scmnd->result = rsp->status; What i can see is that we have other places to de-reference scmnd and scmnd = srp_claim_req(ch, req, NULL, scmnd) is possible to return a NULL to scmnd Thanks Zhijian > > if (rsp->flags & SRP_RSP_FLAG_SNSVALID) { > memcpy(scmnd->sense_buffer, rsp->data + ^^^^
On 2022/8/31 9:59, Li Zhijian wrote: > > What i can see is that we have other places to de-reference scmnd and > > scmnd = srp_claim_req(ch, req, NULL, scmnd) is possible to return a NULL > to scmnd Hi, Thanks for your review. Yes, it seems better to just check scmnd before setting scmnd->result, like this: diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 7720ea270ed8..99f5e7f852b3 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1972,7 +1972,9 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) return; } - scmnd->result = rsp->status; + + if (scmnd) + scmnd->result = rsp->status; Best Regards, Xiao Yang
On 8/30/22 18:47, yangx.jy@fujitsu.com wrote: > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > index 7720ea270ed8..528cdd0daba4 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) > if (scmnd) { > req = scsi_cmd_priv(scmnd); > scmnd = srp_claim_req(ch, req, NULL, scmnd); > + scmnd->result = rsp->status; > } else { > shost_printk(KERN_ERR, target->scsi_host, > "Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n", > @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) > > return; > } > - scmnd->result = rsp->status; > > if (rsp->flags & SRP_RSP_FLAG_SNSVALID) { > memcpy(scmnd->sense_buffer, rsp->data + Since there is a 'return' statement in the else branch, I don't see how this patch can make a difference? Thanks, Bart.
On 31/08/2022 10:31, Yang, Xiao/杨 晓 wrote: > On 2022/8/31 9:59, Li Zhijian wrote: >> What i can see is that we have other places to de-reference scmnd and >> >> scmnd = srp_claim_req(ch, req, NULL, scmnd) is possible to return a NULL >> to scmnd > Hi, > > Thanks for your review. > > Yes, it seems better to just check scmnd before setting scmnd->result, > like this: > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c > b/drivers/infiniband/ulp/srp/ib_srp.c > index 7720ea270ed8..99f5e7f852b3 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -1972,7 +1972,9 @@ static void srp_process_rsp(struct srp_rdma_ch > *ch, struct srp_rsp *rsp) > > return; > } > - scmnd->result = rsp->status; > + > + if (scmnd) > + scmnd->result = rsp->status; Not really, i think you may need to return directly if !scmnd > > Best Regards, > Xiao Yang
On 2022/8/31 10:47, Bart Van Assche wrote: > On 8/30/22 18:47, yangx.jy@fujitsu.com wrote: >> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c >> b/drivers/infiniband/ulp/srp/ib_srp.c >> index 7720ea270ed8..528cdd0daba4 100644 >> --- a/drivers/infiniband/ulp/srp/ib_srp.c >> +++ b/drivers/infiniband/ulp/srp/ib_srp.c >> @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch >> *ch, struct srp_rsp *rsp) >> if (scmnd) { >> req = scsi_cmd_priv(scmnd); >> scmnd = srp_claim_req(ch, req, NULL, scmnd); >> + scmnd->result = rsp->status; >> } else { >> shost_printk(KERN_ERR, target->scsi_host, >> "Null scmnd for RSP w/tag %#016llx received on >> ch %td / QP %#x\n", >> @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch >> *ch, struct srp_rsp *rsp) >> return; >> } >> - scmnd->result = rsp->status; >> if (rsp->flags & SRP_RSP_FLAG_SNSVALID) { >> memcpy(scmnd->sense_buffer, rsp->data + > > Since there is a 'return' statement in the else branch, I don't see how > this patch can make a difference? Hi Bart, Sorry, I didn't make the right fix. I will send v2 patch. I think scmnd may be set to NULL after srp_claim_req() is called and then setting scmnd->result can trigger the NULL pointer dereference. Best Regards, Xiao Yang > > Thanks, > > Bart.
On 8/30/22 20:16, yangx.jy@fujitsu.com wrote: > Sorry, I didn't make the right fix. I will send v2 patch. > I think scmnd may be set to NULL after srp_claim_req() is called and > then setting scmnd->result can trigger the NULL pointer dereference. Something like this untested patch may be what you are looking for: diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 7720ea270ed8..d7f69e593a63 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1961,7 +1961,8 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) if (scmnd) { req = scsi_cmd_priv(scmnd); scmnd = srp_claim_req(ch, req, NULL, scmnd); - } else { + } + if (!scmnd) { shost_printk(KERN_ERR, target->scsi_host, "Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n", rsp->tag, ch - target->ch, ch->qp->qp_num); Bart.
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 7720ea270ed8..528cdd0daba4 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) if (scmnd) { req = scsi_cmd_priv(scmnd); scmnd = srp_claim_req(ch, req, NULL, scmnd); + scmnd->result = rsp->status; } else { shost_printk(KERN_ERR, target->scsi_host, "Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n", @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp) return; } - scmnd->result = rsp->status; if (rsp->flags & SRP_RSP_FLAG_SNSVALID) { memcpy(scmnd->sense_buffer, rsp->data +
This change fixes the following kernel NULL pointer dereference which is reproduced by blktests srp/007 occasionally. BUG: kernel NULL pointer dereference, address: 0000000000000170 #PF: supervisor write access in kernel mode #PF: error_code(0x0002) - not-present page PGD 0 P4D 0 Oops: 0002 [#1] PREEMPT SMP NOPTI CPU: 0 PID: 9 Comm: kworker/0:1H Kdump: loaded Not tainted 6.0.0-rc1+ #37 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-29-g6a62e0cb0dfe-prebuilt.qemu.org 04/01/2014 Workqueue: 0x0 (kblockd) RIP: 0010:srp_recv_done+0x176/0x500 [ib_srp] Code: 00 4d 85 ff 0f 84 52 02 00 00 48 c7 82 80 02 00 00 00 00 00 00 4c 89 df 4c 89 14 24 e8 53 d3 4a f6 4c 8b 14 24 41 0f b6 42 13 <41> 89 87 70 01 00 00 41 0f b6 52 12 f6 c2 02 74 44 41 8b 42 1c b9 RSP: 0018:ffffaef7c0003e28 EFLAGS: 00000282 RAX: 0000000000000000 RBX: ffff9bc9486dea60 RCX: 0000000000000000 RDX: 0000000000000102 RSI: ffffffffb76bbd0e RDI: 00000000ffffffff RBP: ffff9bc980099a00 R08: 0000000000000001 R09: 0000000000000001 R10: ffff9bca53ef0000 R11: ffff9bc980099a10 R12: ffff9bc956e14000 R13: ffff9bc9836b9cb0 R14: ffff9bc9557b4480 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff9bc97ec00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000170 CR3: 0000000007e04000 CR4: 00000000000006f0 Call Trace: <IRQ> __ib_process_cq+0xb7/0x280 [ib_core] ib_poll_handler+0x2b/0x130 [ib_core] irq_poll_softirq+0x93/0x150 __do_softirq+0xee/0x4b8 irq_exit_rcu+0xf7/0x130 sysvec_apic_timer_interrupt+0x8e/0xc0 </IRQ> Fixes: aef9ec39c47f ("IB: Add SCSI RDMA Protocol (SRP) initiator") Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> --- drivers/infiniband/ulp/srp/ib_srp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)