[ofa-general] Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

[Bart Van Assche]

On Thu, Aug 6, 2009 at 7:41 PM, Roland Dreier<rdreier at cisco.com> wrote:
>
>  > Specifically scmnd->host_scribble can just be Zero.
>
> I see at last, thanks!
>
> The issue is that SRP is using host_scribble to hold an index, and index
> 0 is valid for us.
>
> I guess the fix is a bit complex, but basically we should use
> host_scribble to point to the request, and if we don't find a request in
> reset_device we should allocate one.

A fix like the one below ?

--- linux-2.6.30.4/drivers/infiniband/ulp/srp/ib_srp-orig.c	2009-08-03
12:13:11.000000000 +0200
+++ linux-2.6.30.4/drivers/infiniband/ulp/srp/ib_srp.c	2009-08-07
10:23:27.000000000 +0200
@@ -1371,16 +1371,27 @@ out:
 	return -1;
 }

+/**
+ * Look up the struct srp_request that has been associated with the specified
+ * SCSI command by srp_queuecommand().
+ *
+ * Returns 0 upon success and -1 upon failure.
+ */
 static int srp_find_req(struct srp_target_port *target,
 			struct scsi_cmnd *scmnd,
 			struct srp_request **req)
 {
-	if (scmnd->host_scribble == (void *) -1L)
-		return -1;
+	/*
+	 * The code below will only work if SRP_RQ_SIZE is a power of two,
+	 * so check this first.
+	 */
+	BUILD_BUG_ON((SRP_RQ_SIZE ^ (SRP_RQ_SIZE - 1))
+		     != (SRP_RQ_SIZE | (SRP_RQ_SIZE - 1)));

-	*req = &target->req_ring[(long) scmnd->host_scribble];
+	*req = &target->req_ring[(long)scmnd->host_scribble
+				 & (SRP_RQ_SIZE - 1)];

-	return 0;
+	return (*req)->scmnd == scmnd ? 0 : -1;
 }

 static int srp_abort(struct scsi_cmnd *scmnd)
@@ -1423,8 +1434,15 @@ static int srp_reset_device(struct scsi_

 	if (target->qp_in_error)
 		return FAILED;
-	if (srp_find_req(target, scmnd, &req))
-		return FAILED;
+	if (srp_find_req(target, scmnd, &req)) {
+		/*
+		 * scmnd has not yet been queued -- queue it now. This can
+		 * happen e.g. when a SG_SCSI_RESET ioctl has been issued.
+		 */
+		if (srp_queuecommand(scmnd, scmnd->scsi_done)
+		    || srp_find_req(target, scmnd, &req))
+			return FAILED;
+	}
 	if (srp_send_tsk_mgmt(target, req, SRP_TSK_LUN_RESET))
 		return FAILED;
 	if (req->tsk_status)