diff mbox series

[SRU,Xenial,CVE-2018-20836] scsi: libsas: fix a race condition when smp task timeout

Message ID 20190530223113.2593-2-connor.kuehl@canonical.com
State New
Headers show
Series [SRU,Xenial,CVE-2018-20836] scsi: libsas: fix a race condition when smp task timeout | expand

Commit Message

Connor Kuehl May 30, 2019, 10:31 p.m. UTC 
From: Jason Yan <yanaijie@huawei.com>

CVE-2018-20836

BugLink: https://bugs.launchpad.net/bugs/1808912

When the lldd is processing the complete sas task in interrupt and set the
task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to be
triggered at the same time. And smp_task_timedout() will complete the task
wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may freed
before lldd end the interrupt process. Thus a use-after-free will happen.

Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
set. And remove the check of the return value of the del_timer(). Once the
LLDD sets DONE, it must call task->done(), which will call
smp_task_done()->complete() and the task will be completed and freed
correctly.

Reported-by: chenxiang <chenxiang66@hisilicon.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: John Garry <john.garry@huawei.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
(cherry picked from commit b90cd6f2b905905fb42671009dc0e27c310a16ae)
Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>
---
 drivers/scsi/libsas/sas_expander.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Andrea Righi May 31, 2019, 7:45 a.m. UTC | #1 
On Thu, May 30, 2019 at 03:31:13PM -0700, Connor Kuehl wrote:
> From: Jason Yan <yanaijie@huawei.com>
> 
> CVE-2018-20836
> 
> BugLink: https://bugs.launchpad.net/bugs/1808912
> 
> When the lldd is processing the complete sas task in interrupt and set the
> task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to be
> triggered at the same time. And smp_task_timedout() will complete the task
> wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may freed
> before lldd end the interrupt process. Thus a use-after-free will happen.
> 
> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
> set. And remove the check of the return value of the del_timer(). Once the
> LLDD sets DONE, it must call task->done(), which will call
> smp_task_done()->complete() and the task will be completed and freed
> correctly.
> 
> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> (cherry picked from commit b90cd6f2b905905fb42671009dc0e27c310a16ae)
> Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>

Clean upstream cherry pick. Fix already applied to the other kernel
relases. Looks good.

Acked-by: Andrea Righi <andrea.righi@canonical.com>
Po-Hsu Lin June 4, 2019, 6:29 a.m. UTC | #2 
Clean cherry-pick, series nominated in the launchpad bug.
Acked-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
Kleber Sacilotto de Souza June 4, 2019, 2:20 p.m. UTC | #3 
On 5/31/19 12:31 AM, Connor Kuehl wrote:
> From: Jason Yan <yanaijie@huawei.com>
> 
> CVE-2018-20836
> 
> BugLink: https://bugs.launchpad.net/bugs/1808912

Hi Connor,

You have added the above BugLink as well for this CVE fix but this bug
report has been marked as invalid for Xenial.

Generally we don't need to add a BugLink for a CVE fix, the CVE ID
is enough. Is this BugLink really needed?


Thanks,
Kleber

> 
> When the lldd is processing the complete sas task in interrupt and set the
> task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to be
> triggered at the same time. And smp_task_timedout() will complete the task
> wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may freed
> before lldd end the interrupt process. Thus a use-after-free will happen.
> 
> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
> set. And remove the check of the return value of the del_timer(). Once the
> LLDD sets DONE, it must call task->done(), which will call
> smp_task_done()->complete() and the task will be completed and freed
> correctly.
> 
> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> (cherry picked from commit b90cd6f2b905905fb42671009dc0e27c310a16ae)
> Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>
> ---
>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 7be581f7c35d..1a6f65db615e 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -47,17 +47,16 @@ static void smp_task_timedout(unsigned long _task)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&task->task_state_lock, flags);
> -	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
> +	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>  		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> +		complete(&task->slow_task->completion);
> +	}
>  	spin_unlock_irqrestore(&task->task_state_lock, flags);
> -
> -	complete(&task->slow_task->completion);
>  }
>  
>  static void smp_task_done(struct sas_task *task)
>  {
> -	if (!del_timer(&task->slow_task->timer))
> -		return;
> +	del_timer(&task->slow_task->timer);
>  	complete(&task->slow_task->completion);
>  }
>  
>
Connor Kuehl June 4, 2019, 2:38 p.m. UTC | #4 
On 6/4/19 7:20 AM, Kleber Souza wrote:
> On 5/31/19 12:31 AM, Connor Kuehl wrote:
>> From: Jason Yan <yanaijie@huawei.com>
>>
>> CVE-2018-20836
>>
>> BugLink: https://bugs.launchpad.net/bugs/1808912
> 
> Hi Connor,
> 
> You have added the above BugLink as well for this CVE fix but this bug
> report has been marked as invalid for Xenial.
> 
> Generally we don't need to add a BugLink for a CVE fix, the CVE ID
> is enough. Is this BugLink really needed?

No, sorry, my mistake. The BugLink isn't needed.

Connor

> 
> 
> Thanks,
> Kleber
> 
>>
>> When the lldd is processing the complete sas task in interrupt and set the
>> task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to be
>> triggered at the same time. And smp_task_timedout() will complete the task
>> wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may freed
>> before lldd end the interrupt process. Thus a use-after-free will happen.
>>
>> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
>> set. And remove the check of the return value of the del_timer(). Once the
>> LLDD sets DONE, it must call task->done(), which will call
>> smp_task_done()->complete() and the task will be completed and freed
>> correctly.
>>
>> Reported-by: chenxiang <chenxiang66@hisilicon.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>> Reviewed-by: John Garry <john.garry@huawei.com>
>> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
>> (cherry picked from commit b90cd6f2b905905fb42671009dc0e27c310a16ae)
>> Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>
>> ---
>>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>> index 7be581f7c35d..1a6f65db615e 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -47,17 +47,16 @@ static void smp_task_timedout(unsigned long _task)
>>  	unsigned long flags;
>>  
>>  	spin_lock_irqsave(&task->task_state_lock, flags);
>> -	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
>> +	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>>  		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
>> +		complete(&task->slow_task->completion);
>> +	}
>>  	spin_unlock_irqrestore(&task->task_state_lock, flags);
>> -
>> -	complete(&task->slow_task->completion);
>>  }
>>  
>>  static void smp_task_done(struct sas_task *task)
>>  {
>> -	if (!del_timer(&task->slow_task->timer))
>> -		return;
>> +	del_timer(&task->slow_task->timer);
>>  	complete(&task->slow_task->completion);
>>  }
>>  
>>
>
Kleber Sacilotto de Souza June 5, 2019, 10:38 a.m. UTC | #5 
On 5/31/19 12:31 AM, Connor Kuehl wrote:
> From: Jason Yan <yanaijie@huawei.com>
> 
> CVE-2018-20836
> 
> BugLink: https://bugs.launchpad.net/bugs/1808912
> 
> When the lldd is processing the complete sas task in interrupt and set the
> task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to be
> triggered at the same time. And smp_task_timedout() will complete the task
> wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may freed
> before lldd end the interrupt process. Thus a use-after-free will happen.
> 
> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
> set. And remove the check of the return value of the del_timer(). Once the
> LLDD sets DONE, it must call task->done(), which will call
> smp_task_done()->complete() and the task will be completed and freed
> correctly.
> 
> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> (cherry picked from commit b90cd6f2b905905fb42671009dc0e27c310a16ae)
> Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>
> ---
>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 7be581f7c35d..1a6f65db615e 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -47,17 +47,16 @@ static void smp_task_timedout(unsigned long _task)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&task->task_state_lock, flags);
> -	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
> +	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>  		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> +		complete(&task->slow_task->completion);
> +	}
>  	spin_unlock_irqrestore(&task->task_state_lock, flags);
> -
> -	complete(&task->slow_task->completion);
>  }
>  
>  static void smp_task_done(struct sas_task *task)
>  {
> -	if (!del_timer(&task->slow_task->timer))
> -		return;
> +	del_timer(&task->slow_task->timer);
>  	complete(&task->slow_task->completion);
>  }
>  
> 

This patch has already been applied to Xenial as part of
the update to 4.4.180 upstream stable release (LP: #1830176).

Thanks,
Kleber
diff mbox series

Patch

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 7be581f7c35d..1a6f65db615e 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -47,17 +47,16 @@  static void smp_task_timedout(unsigned long _task)
 	unsigned long flags;
 
 	spin_lock_irqsave(&task->task_state_lock, flags);
-	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
+	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
 		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
+		complete(&task->slow_task->completion);
+	}
 	spin_unlock_irqrestore(&task->task_state_lock, flags);
-
-	complete(&task->slow_task->completion);
 }
 
 static void smp_task_done(struct sas_task *task)
 {
-	if (!del_timer(&task->slow_task->timer))
-		return;
+	del_timer(&task->slow_task->timer);
 	complete(&task->slow_task->completion);
 }