diff mbox series

scsi: target: tcmu: Fix a use after free in tcmu_check_expired_queue_cmd()

Message ID 20200523101129.GB98132@mwanda (mailing list archive)
State Accepted
Commit 9d7464b18892332e35ff37f0b024429a1a9835e6
Headers show
Series scsi: target: tcmu: Fix a use after free in tcmu_check_expired_queue_cmd() | expand

Commit Message

Dan Carpenter May 23, 2020, 10:11 a.m. UTC
The pr_debug() dereferences "cmd" after we already freed it by calling
tcmu_free_cmd(cmd).  The debug printk needs to be done earlier.

Fixes: 61fb24822166 ("scsi: target: tcmu: Userspace must not complete queued commands")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/target/target_core_user.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

David Disseldorp May 23, 2020, 1:03 p.m. UTC | #1
On Sat, 23 May 2020 13:11:29 +0300, Dan Carpenter wrote:

> The pr_debug() dereferences "cmd" after we already freed it by calling
> tcmu_free_cmd(cmd).  The debug printk needs to be done earlier.
> 
> Fixes: 61fb24822166 ("scsi: target: tcmu: Userspace must not complete queued commands")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: David Disseldorp <ddiss@suse.de>

Looks like this could be squashed in with the change that it fixes,
given that 5.8/scsi-queue hasn't gone out yet.

Cheers, David
Mike Christie May 23, 2020, 7:04 p.m. UTC | #2
On 5/23/20 5:11 AM, Dan Carpenter wrote:
> The pr_debug() dereferences "cmd" after we already freed it by calling
> tcmu_free_cmd(cmd).  The debug printk needs to be done earlier.
> 
> Fixes: 61fb24822166 ("scsi: target: tcmu: Userspace must not complete queued commands")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/target/target_core_user.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 904d8a8373f2..28fb9441de7a 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1292,13 +1292,13 @@ static void tcmu_check_expired_queue_cmd(struct tcmu_cmd *cmd)
>  	if (!time_after(jiffies, cmd->deadline))
>  		return;
>  
> +	pr_debug("Timing out queued cmd %p on dev %s.\n",
> +		  cmd, cmd->tcmu_dev->name);
> +
>  	list_del_init(&cmd->queue_entry);
>  	se_cmd = cmd->se_cmd;
>  	tcmu_free_cmd(cmd);
>  
> -	pr_debug("Timing out queued cmd %p on dev %s.\n",
> -		  cmd, cmd->tcmu_dev->name);
> -
>  	target_complete_cmd(se_cmd, SAM_STAT_TASK_SET_FULL);
>  }
>  

Thanks.

Reviewed-by: Mike Christie <mchristi@redhat.com>
Bodo Stroesser May 25, 2020, 9:11 a.m. UTC | #3
On 05/23/20 12:11, Dan Carpenter wrote:
> The pr_debug() dereferences "cmd" after we already freed it by calling
> tcmu_free_cmd(cmd).  The debug printk needs to be done earlier.
> 
> Fixes: 61fb24822166 ("scsi: target: tcmu: Userspace must not complete queued commands")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/target/target_core_user.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

Thank you.

I'm very sorry for this stupid bug.

BR, Bodo
Dan Carpenter May 26, 2020, 12:26 p.m. UTC | #4
On Mon, May 25, 2020 at 11:11:38AM +0200, Bodo Stroesser wrote:
> On 05/23/20 12:11, Dan Carpenter wrote:
> > The pr_debug() dereferences "cmd" after we already freed it by calling
> > tcmu_free_cmd(cmd).  The debug printk needs to be done earlier.
> > 
> > Fixes: 61fb24822166 ("scsi: target: tcmu: Userspace must not complete queued commands")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   drivers/target/target_core_user.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> 
> Thank you.
> 
> I'm very sorry for this stupid bug.

Bugs like this are super common.  Part of being human.  It would have
been hard to discover via testing because you have disable memory
poisoning on free and debugging at the same time.

regards,
dan carpenter
Martin K. Petersen May 27, 2020, 2:12 a.m. UTC | #5
On Sat, 23 May 2020 13:11:29 +0300, Dan Carpenter wrote:

> The pr_debug() dereferences "cmd" after we already freed it by calling
> tcmu_free_cmd(cmd).  The debug printk needs to be done earlier.

Applied to 5.8/scsi-queue, thanks!

[1/1] scsi: target: tcmu: Fix a use after free in tcmu_check_expired_queue_cmd()
      https://git.kernel.org/mkp/scsi/c/9d7464b18892
diff mbox series

Patch

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 904d8a8373f2..28fb9441de7a 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1292,13 +1292,13 @@  static void tcmu_check_expired_queue_cmd(struct tcmu_cmd *cmd)
 	if (!time_after(jiffies, cmd->deadline))
 		return;
 
+	pr_debug("Timing out queued cmd %p on dev %s.\n",
+		  cmd, cmd->tcmu_dev->name);
+
 	list_del_init(&cmd->queue_entry);
 	se_cmd = cmd->se_cmd;
 	tcmu_free_cmd(cmd);
 
-	pr_debug("Timing out queued cmd %p on dev %s.\n",
-		  cmd, cmd->tcmu_dev->name);
-
 	target_complete_cmd(se_cmd, SAM_STAT_TASK_SET_FULL);
 }