[PATCH #upstream-fixes] libata: fix NULL sdev dereference race in atapi_qc_complete()

[PATCH #upstream-fixes] libata: fix NULL sdev dereference race in atapi_qc_complete()

[Tejun Heo]

SCSI commands may be issued between __scsi_add_device() and dev->sdev
assignment, so it's unsafe for ata_qc_complete() to dereference
dev->sdev->locked without checking whether it's NULL or not.  Fix it.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: stable@kernel.org
---
I actually hit this race condition.  Lucky me.  :-)

Thanks.

 drivers/ata/libata-scsi.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -2552,8 +2552,11 @@ static void atapi_qc_complete(struct ata
 		 *
 		 * If door lock fails, always clear sdev->locked to
 		 * avoid this infinite loop.
+		 *
+		 * This may happen before SCSI scan is complete.  Make
+		 * sure qc->dev->sdev isn't NULL before dereferencing.
 		 */
-		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL)
+		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
 			qc->dev->sdev->locked = 0;

 		qc->scsicmd->result = SAM_STAT_CHECK_CONDITION;

Re: [PATCH #upstream-fixes] libata: fix NULL sdev dereference race in atapi_qc_complete()

[Mark Lord]

On 10-11-01 06:39 AM, Tejun Heo wrote:
> SCSI commands may be issued between __scsi_add_device() and dev->sdev
> assignment, so it's unsafe for ata_qc_complete() to dereference
> dev->sdev->locked without checking whether it's NULL or not.  Fix it.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: stable@kernel.org
> ---
> I actually hit this race condition.  Lucky me.  :-)
>
> Thanks.
>
>   drivers/ata/libata-scsi.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: work/drivers/ata/libata-scsi.c
> ===================================================================
> --- work.orig/drivers/ata/libata-scsi.c
> +++ work/drivers/ata/libata-scsi.c
> @@ -2552,8 +2552,11 @@ static void atapi_qc_complete(struct ata
>   		 *
>   		 * If door lock fails, always clear sdev->locked to
>   		 * avoid this infinite loop.
> +		 *
> +		 * This may happen before SCSI scan is complete.  Make
> +		 * sure qc->dev->sdev isn't NULL before dereferencing.
>   		 */
> -		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL)
> +		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL&&  qc->dev->sdev)
>   			qc->dev->sdev->locked = 0;
>
>   		qc->scsicmd->result = SAM_STAT_CHECK_CONDITION;


Mmmm.. for some reason, this just screams "band-aid" to me,
and makes me worry deeply about the underlaying race condition
it all suggests..

James?

Re: [PATCH #upstream-fixes] libata: fix NULL sdev dereference race in atapi_qc_complete()

[Tejun Heo]

Hello, Mark.

On 11/02/2010 10:31 PM, Mark Lord wrote:
> On 10-11-01 06:39 AM, Tejun Heo wrote:
>> SCSI commands may be issued between __scsi_add_device() and dev->sdev
>> assignment, so it's unsafe for ata_qc_complete() to dereference
>> dev->sdev->locked without checking whether it's NULL or not.  Fix it.
>>
>> Signed-off-by: Tejun Heo<tj@kernel.org>
>> Cc: stable@kernel.org
> 
> Mmmm.. for some reason, this just screams "band-aid" to me,
> and makes me worry deeply about the underlaying race condition
> it all suggests..

Yeah, the coupling between sdev and ata_dev may look somewhat
band-aidy but AFAICS all others are explicitly checking whether
dev->sdev is set.  It's somewhat inevitable given the current probing
sequence (ATA dev comes up first and then tells SCSI to probe itself).

Thanks.

-- 
tejun