[PATCH] scsi_scan.c: bug fix: starget use after free issue

[PATCH] scsi_scan.c: bug fix: starget use after free issue

[James Smart]

When reaping the starget, after all sdev's have been removed, the starget
was queued for deletion via usercontext, but was left on the shost's
__targets list. Another scanning thread can match the starget and use it, 
causing reference after free problems.

This patch unlinks the starget at the same time it is scheduled for deletion.

-- james s


Signed-off-by: James Smart <james.smart@emulex.com>

diff -upNr a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	2006-06-14 11:37:09.000000000 -0400
+++ b/drivers/scsi/scsi_scan.c	2006-06-14 16:55:58.000000000 -0400
@@ -415,7 +415,6 @@ static void scsi_target_reap_usercontext
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->hostt->target_destroy)
 		shost->hostt->target_destroy(starget);
-	list_del_init(&starget->siblings);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	put_device(&starget->dev);
 }
@@ -439,6 +438,7 @@ void scsi_target_reap(struct scsi_target
 	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
 		BUG_ON(starget->state == STARGET_DEL);
 		starget->state = STARGET_DEL;
+		list_del_init(&starget->siblings);
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		execute_in_process_context(scsi_target_reap_usercontext,
 					   starget, &starget->ew);

Re: [PATCH] scsi_scan.c: bug fix: starget use after free issue

[James Bottomley]

On Thu, 2006-06-15 at 12:55 -0400, James Smart wrote:
> When reaping the starget, after all sdev's have been removed, the starget
> was queued for deletion via usercontext, but was left on the shost's
> __targets list. Another scanning thread can match the starget and use it, 
> causing reference after free problems.
> 
> This patch unlinks the starget at the same time it is scheduled for deletion.
> 
This cannot be done this way.  The problem it will introduce is that
we'll think the target has gone and possibly reallocate its name before
device_del is called on it (which means if the new device gets added, it
will return -EEXIST and everything will go wrong).

Where is the actual reference coming from ... perhaps the using place
should simply be checking the state.

James

Re: [PATCH] scsi_scan.c: bug fix: starget use after free issue

[James Smart]

And we've already seen this, even without this change - e.g. the target was
2/3's torn down when we created a new one....

what this really sounds like then is that we need to stop the teardown and
reuse the structure - or - figure out a way to make the kobj teardown happen
sooner so that we reuse the namespace (or disconnect the namespace and the
structures).

-- james

James Bottomley wrote:
> On Thu, 2006-06-15 at 12:55 -0400, James Smart wrote:
>> When reaping the starget, after all sdev's have been removed, the starget
>> was queued for deletion via usercontext, but was left on the shost's
>> __targets list. Another scanning thread can match the starget and use it, 
>> causing reference after free problems.
>>
>> This patch unlinks the starget at the same time it is scheduled for deletion.
>>
> This cannot be done this way.  The problem it will introduce is that
> we'll think the target has gone and possibly reallocate its name before
> device_del is called on it (which means if the new device gets added, it
> will return -EEXIST and everything will go wrong).
> 
> Where is the actual reference coming from ... perhaps the using place
> should simply be checking the state.
> 
> James
> 
> 
> 

Re: [PATCH] scsi_scan.c: bug fix: starget use after free issue

[James Smart]

FYI: This patch is NACK'd as it is superceeded by a patch proposed long
ago (2/23/06), which finally showed up in 2.6.17-rc1. See:
http://marc.theaimsgroup.com/?l=linux-scsi&m=114072663121857&w=2

Note: this patch was corrected the reuse error w/o issue, but did not
address the reuse of the sysfs namespace. The other patch addresses
both.

-- james s

James Smart wrote:
> When reaping the starget, after all sdev's have been removed, the starget
> was queued for deletion via usercontext, but was left on the shost's
> __targets list. Another scanning thread can match the starget and use it, 
> causing reference after free problems.
> 
> This patch unlinks the starget at the same time it is scheduled for deletion.
> 
> -- james s
> 
> 
> Signed-off-by: James Smart <james.smart@emulex.com>
> 
> diff -upNr a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> --- a/drivers/scsi/scsi_scan.c	2006-06-14 11:37:09.000000000 -0400
> +++ b/drivers/scsi/scsi_scan.c	2006-06-14 16:55:58.000000000 -0400
> @@ -415,7 +415,6 @@ static void scsi_target_reap_usercontext
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	if (shost->hostt->target_destroy)
>  		shost->hostt->target_destroy(starget);
> -	list_del_init(&starget->siblings);
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  	put_device(&starget->dev);
>  }
> @@ -439,6 +438,7 @@ void scsi_target_reap(struct scsi_target
>  	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
>  		BUG_ON(starget->state == STARGET_DEL);
>  		starget->state = STARGET_DEL;
> +		list_del_init(&starget->siblings);
>  		spin_unlock_irqrestore(shost->host_lock, flags);
>  		execute_in_process_context(scsi_target_reap_usercontext,
>  					   starget, &starget->ew);
> 
> 
> -
> 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
>