IB/srp: avoid NULL pointer dereference when processing invalid SRP_RSP

If an SRP target sends an invalid SRP_RSP information unit to the SRP initiator this can cause a NULL pointer dereference on the initiator system. This patch avoids such NULL pointer dereferences and makes sure the SRP inititator keeps working.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

drivers/infiniband/ulp/srp/ib_srp.c +3 -1

index ed3f9eb..330452c 100644
@@ -834,10 +834,12 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 		complete(&req->done);
 	} else {
 		scmnd = req->scmnd;
-		if (!scmnd)
+		if (!scmnd) {
 			shost_printk(KERN_ERR, target->scsi_host,
 				     "Null scmnd for RSP w/tag %016llx\n",
 				     (unsigned long long) rsp->tag);
+			goto out_unlock;
+		}
 		scmnd->result = rsp->status;
 
 		if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {

Quoted 4 lines · Bart Van Assche

If an SRP target sends an invalid SRP_RSP information unit to the SRP initiator this can cause a NULL pointer dereference on the initiator system. This patch avoids such NULL pointer dereferences and makes sure the SRP inititator keeps working.

While I'm all for increasing the robustness of the initiator, I'm curious how you would hit this -- I don't see where we would send a SRP request without an associated SCSI command.

I'm also not sure you're doing the right thing here -- you seem to skip the cleanup of the associated SRP request. I suppose that may be handled for you if/when srp_abort() gets called for a missed SCSI request, but again, we should never be in the situation where a request doesn't have a SCSI command. And if srp_abort() doesn't get called, we've leaked a request and possibly a DMA mapping, no?

Quoted 10 lines · David Dillow

While I'm all for increasing the robustness of the initiator, I'm curious how you would hit this -- I don't see where we would send a SRP request without an associated SCSI command.

I'm also not sure you're doing the right thing here -- you seem to skip the cleanup of the associated SRP request. I suppose that may be handled for you if/when srp_abort() gets called for a missed SCSI request, but again, we should never be in the situation where a request doesn't have a SCSI command. And if srp_abort() doesn't get called, we've leaked a request and possibly a DMA mapping, no?

This condition was hit accidentally while testing newly added target code that sent an SRP_RSP not associated with an SRP_CMD or SRP_TSK_MGMT information unit. Since I was surprised to find a test of the scmnd pointer in ib_srp.c followed by an unconditional dereference of it, I coded up this patch.

The target code that triggered this condition has been fixed by now and even was never committed to its source code repository, so this patch isn't important to me.

Regarding leaks: isn't there already a leak in the current ib_srp code if any information unit with another opcode than SRP_RSP is received ?