Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

[Bart Van Assche]

On Wed, Aug 5, 2009 at 10:37 PM, James
Bottomley<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2009-08-05 at 19:54 +0200, Bart Van Assche wrote:
>> On Wed, Aug 5, 2009 at 7:44 PM, Roland Dreier<rdreier@cisco.com> wrote:
>> >
>> >  > The NULL pointer dereference happens when srp_reset_device() calls
>> >  > srp_send_tsk_mgmt(target, req, SRP_TSK_LUN_RESET) with
>> >  > req->scmnd->device == NULL. When the sg_reset command issues an
>> >  > SG_SCSI_RESET ioctl, scsi_reset_provider() is invoked and allocates an
>> >  > scmnd structure and sets scmnd->device to NULL. It is this scmnd
>> >  > structure that is passed to srp_reset_device(). What I'm not sure
>> >  > about is whether scsi_reset_provider() should set req->scmnd->device
>> >  > to a non-NULL value or whether srp_send_tsk_mgmt() should be able to
>> >  > handle the condition req->scmnd->device == NULL.
>> >
>> > Well, I don't see how the reset ioctl can do anything useful unless it
>> > passes a device in with the scsi command -- otherwise for example
>> > srp_reset_device() has no idea what LUN to try and reset.
>>
>> (added linux-scsi in CC)
>>
>> I hope one of the SCSI people can tell us whether the behavior that
>> scsi_reset_provider()
>> passes the value NULL in req->scmnd->device to
>> scsi_try_bus_device_reset() is correct ?
>
> Need more information.
>
> cmd->device is supposed to be initialised in scsi_get_command(), which
> scsi_reset_provider() calls ... why do you think it got set to null?

This thread started with the observation that it is easy to trigger a
NULL pointer dereference in the SRP initiator
(http://bugzilla.kernel.org/show_bug.cgi?id=13893). The following
sequence is sufficient:
* Remove the ib_srp kernel module (doing so closes all active SRP sessions).
* Insert the ib_srp kernel module.
* Create a new SRP connection.
* Issue the sg_reset -d ${srp_device} command in a shell.
The sg_reset command issues an SG_SCSI_RESET ioctl. This ioctl is
processed by invoking scsi_reset_provider(), which in turns invokes
the eh_device_reset_handler method of the SRP initiator. Further
analysis showed that scsi_reset_provider() passes a non-NULL
cmd->device pointer to the SRP initiator, but that the SRP initiator
does not use this value. Instead srp_find_req() looks up a struct
srp_request pointer based on the struct scsi_cmnd * argument and
continues with the struct scsi_cmnd pointer contained in the struct
srp_request.

While I'm not sure that the patch below makes any sense, it makes the
NULL pointer dereference disappear. This made me wonder which
assumptions srp_find_req() is based on ?

--- 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-06
08:50:30.000000000 +0200
@@ -1325,16 +1325,19 @@ static int srp_cm_handler(struct ib_cm_i
 }

 static int srp_send_tsk_mgmt(struct srp_target_port *target,
+                            struct scsi_cmnd *scmnd,
                             struct srp_request *req, u8 func)
 {
        struct srp_iu *iu;
        struct srp_tsk_mgmt *tsk_mgmt;

+       BUG_ON(!scmnd->device);
+
        spin_lock_irq(target->scsi_host->host_lock);

        if (target->state == SRP_TARGET_DEAD ||
            target->state == SRP_TARGET_REMOVED) {
-               req->scmnd->result = DID_BAD_TARGET << 16;
+               scmnd->result = DID_BAD_TARGET << 16;
                goto out;
        }

@@ -1348,7 +1351,7 @@ static int srp_send_tsk_mgmt(struct srp_
        memset(tsk_mgmt, 0, sizeof *tsk_mgmt);

        tsk_mgmt->opcode        = SRP_TSK_MGMT;
-       tsk_mgmt->lun           = cpu_to_be64((u64)
req->scmnd->device->lun << 48);
+       tsk_mgmt->lun           = cpu_to_be64((u64) scmnd->device->lun << 48);
        tsk_mgmt->tag           = req->index | SRP_TAG_TSK_MGMT;
        tsk_mgmt->tsk_mgmt_func = func;
        tsk_mgmt->task_tag      = req->index;
@@ -1395,7 +1398,7 @@ static int srp_abort(struct scsi_cmnd *s
                return FAILED;
        if (srp_find_req(target, scmnd, &req))
                return FAILED;
-       if (srp_send_tsk_mgmt(target, req, SRP_TSK_ABORT_TASK))
+       if (srp_send_tsk_mgmt(target, scmnd, req, SRP_TSK_ABORT_TASK))
                return FAILED;

        spin_lock_irq(target->scsi_host->host_lock);
@@ -1425,7 +1428,9 @@ static int srp_reset_device(struct scsi_
                return FAILED;
        if (srp_find_req(target, scmnd, &req))
                return FAILED;
-       if (srp_send_tsk_mgmt(target, req, SRP_TSK_LUN_RESET))
+       if (WARN_ON(!scmnd->device))
+               return FAILED;
+       if (srp_send_tsk_mgmt(target, scmnd, req, SRP_TSK_LUN_RESET))
                return FAILED;
        if (req->tsk_status)
                return FAILED;

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

[Boaz Harrosh]

On 08/06/2009 10:39 AM, Bart Van Assche wrote:
> On Wed, Aug 5, 2009 at 10:37 PM, James
> Bottomley<James.Bottomley@hansenpartnership.com> wrote:
>> On Wed, 2009-08-05 at 19:54 +0200, Bart Van Assche wrote:
>>> On Wed, Aug 5, 2009 at 7:44 PM, Roland Dreier<rdreier@cisco.com> wrote:
>>>>
>>>>  > The NULL pointer dereference happens when srp_reset_device() calls
>>>>  > srp_send_tsk_mgmt(target, req, SRP_TSK_LUN_RESET) with
>>>>  > req->scmnd->device == NULL. When the sg_reset command issues an
>>>>  > SG_SCSI_RESET ioctl, scsi_reset_provider() is invoked and allocates an
>>>>  > scmnd structure and sets scmnd->device to NULL. It is this scmnd
>>>>  > structure that is passed to srp_reset_device(). What I'm not sure
>>>>  > about is whether scsi_reset_provider() should set req->scmnd->device
>>>>  > to a non-NULL value or whether srp_send_tsk_mgmt() should be able to
>>>>  > handle the condition req->scmnd->device == NULL.
>>>>
>>>> Well, I don't see how the reset ioctl can do anything useful unless it
>>>> passes a device in with the scsi command -- otherwise for example
>>>> srp_reset_device() has no idea what LUN to try and reset.
>>>
>>> (added linux-scsi in CC)
>>>
>>> I hope one of the SCSI people can tell us whether the behavior that
>>> scsi_reset_provider()
>>> passes the value NULL in req->scmnd->device to
>>> scsi_try_bus_device_reset() is correct ?
>>
>> Need more information.
>>
>> cmd->device is supposed to be initialised in scsi_get_command(), which
>> scsi_reset_provider() calls ... why do you think it got set to null?
> 
> This thread started with the observation that it is easy to trigger a
> NULL pointer dereference in the SRP initiator
> (http://bugzilla.kernel.org/show_bug.cgi?id=13893). The following
> sequence is sufficient:
> * Remove the ib_srp kernel module (doing so closes all active SRP sessions).
> * Insert the ib_srp kernel module.
> * Create a new SRP connection.
> * Issue the sg_reset -d ${srp_device} command in a shell.
> The sg_reset command issues an SG_SCSI_RESET ioctl. This ioctl is
> processed by invoking scsi_reset_provider(), which in turns invokes
> the eh_device_reset_handler method of the SRP initiator. Further
> analysis showed that scsi_reset_provider() passes a non-NULL
> cmd->device pointer to the SRP initiator, but that the SRP initiator
> does not use this value. Instead srp_find_req() looks up a struct
> srp_request pointer based on the struct scsi_cmnd * argument and
> continues with the struct scsi_cmnd pointer contained in the struct
> srp_request.
> 
> While I'm not sure that the patch below makes any sense, it makes the
> NULL pointer dereference disappear. This made me wonder which
> assumptions srp_find_req() is based on ?
> 

[Just out of memory, I've not inspected the code for a long time]

It looks like an srp_request was never allocated for the reset
command. (since it never went through .queuecommand)

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;

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

	return 0;
}

Specifically scmnd->host_scribble can just be Zero.
When queues are active that does not matter and a device is found
since the reset does not really need the scsi_cmnd. But in above
scenario the queues were never used and the array entry is empty.

Boaz

> --- 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-06
> 08:50:30.000000000 +0200
> @@ -1325,16 +1325,19 @@ static int srp_cm_handler(struct ib_cm_i
>  }
> 
>  static int srp_send_tsk_mgmt(struct srp_target_port *target,
> +                            struct scsi_cmnd *scmnd,
>                              struct srp_request *req, u8 func)
>  {
>         struct srp_iu *iu;
>         struct srp_tsk_mgmt *tsk_mgmt;
> 
> +       BUG_ON(!scmnd->device);
> +
>         spin_lock_irq(target->scsi_host->host_lock);
> 
>         if (target->state == SRP_TARGET_DEAD ||
>             target->state == SRP_TARGET_REMOVED) {
> -               req->scmnd->result = DID_BAD_TARGET << 16;
> +               scmnd->result = DID_BAD_TARGET << 16;
>                 goto out;
>         }
> 
> @@ -1348,7 +1351,7 @@ static int srp_send_tsk_mgmt(struct srp_
>         memset(tsk_mgmt, 0, sizeof *tsk_mgmt);
> 
>         tsk_mgmt->opcode        = SRP_TSK_MGMT;
> -       tsk_mgmt->lun           = cpu_to_be64((u64)
> req->scmnd->device->lun << 48);
> +       tsk_mgmt->lun           = cpu_to_be64((u64) scmnd->device->lun << 48);
>         tsk_mgmt->tag           = req->index | SRP_TAG_TSK_MGMT;
>         tsk_mgmt->tsk_mgmt_func = func;
>         tsk_mgmt->task_tag      = req->index;
> @@ -1395,7 +1398,7 @@ static int srp_abort(struct scsi_cmnd *s
>                 return FAILED;
>         if (srp_find_req(target, scmnd, &req))
>                 return FAILED;
> -       if (srp_send_tsk_mgmt(target, req, SRP_TSK_ABORT_TASK))
> +       if (srp_send_tsk_mgmt(target, scmnd, req, SRP_TSK_ABORT_TASK))
>                 return FAILED;
> 
>         spin_lock_irq(target->scsi_host->host_lock);
> @@ -1425,7 +1428,9 @@ static int srp_reset_device(struct scsi_
>                 return FAILED;
>         if (srp_find_req(target, scmnd, &req))
>                 return FAILED;
> -       if (srp_send_tsk_mgmt(target, req, SRP_TSK_LUN_RESET))
> +       if (WARN_ON(!scmnd->device))
> +               return FAILED;
> +       if (srp_send_tsk_mgmt(target, scmnd, req, SRP_TSK_LUN_RESET))
>                 return FAILED;
>         if (req->tsk_status)
>                 return FAILED;
> 
> Bart.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On Thu, Aug 6, 2009 at 10:30 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> [Just out of memory, I've not inspected the code for a long time]
>
> It looks like an srp_request was never allocated for the reset
> command. (since it never went through .queuecommand)
>
> 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;
>
>        *req = &target->req_ring[(long) scmnd->host_scribble];
>
>        return 0;
> }
>
> Specifically scmnd->host_scribble can just be Zero.
> When queues are active that does not matter and a device is found
> since the reset does not really need the scsi_cmnd. But in above
> scenario the queues were never used and the array entry is empty.

Hello Boaz,

Thanks for the info. Do you know by heart which SCSI drivers process
the SG_SCSI_RESET ioctl correctly and that could be used as an example
for fixing the SRP initiator ?

Bart.

Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

[James Bottomley]

On Thu, 2009-08-06 at 17:38 +0200, Bart Van Assche wrote:
> On Thu, Aug 6, 2009 at 10:30 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> > [Just out of memory, I've not inspected the code for a long time]
> >
> > It looks like an srp_request was never allocated for the reset
> > command. (since it never went through .queuecommand)
> >
> > 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;
> >
> >        *req = &target->req_ring[(long) scmnd->host_scribble];
> >
> >        return 0;
> > }
> >
> > Specifically scmnd->host_scribble can just be Zero.
> > When queues are active that does not matter and a device is found
> > since the reset does not really need the scsi_cmnd. But in above
> > scenario the queues were never used and the array entry is empty.
> 
> Hello Boaz,
> 
> Thanks for the info. Do you know by heart which SCSI drivers process
> the SG_SCSI_RESET ioctl correctly and that could be used as an example
> for fixing the SRP initiator ?

Basically all of them which are in regular use for clustering; so SAN:
qla2xxx; lpfc.  And for legacy SPI clusters: aic7xxx;mptspi

James

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

[Roland Dreier]

 > 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.

It's a bit unfortunate that the SCSI midlayer bypasses queueing for the
device reset command because it means we may not have a slot in our
queue for the reset request etc but I suppose that's even more involved
to fix.

 - R.

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@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)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

[Roland Dreier]

 > A fix like the one below ?

I think this gets us part of the way, but not quite.

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

could this be BUILD_BUG_ON(!is_power_of_2(SRP_RQ_SIZE)) ?
 > 
 > -	*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;

I don't think we can just pass the command to srp_queuecommand() here.
For one thing queuecommand requires some locking, and second, we don't
actually want to queue the command -- in fact I'm not sure it is set up
properly with an opcode etc to execute the command.

What I think needs to happen is we need to allocate a request for the
command the same way srp_queuecommand() does, and in fact maybe that
code could be factored out to avoid duplication.

 -R .