linux-kernel - Re: [PATCH] target/user: Fix use-after-free cmd->se

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 4 Jan 2017 16:51:49 +0800
From:   Xiubo Li <lixiubo@...s.chinamobile.com>
To:     Mike Christie <mchristi@...hat.com>, bart.vanassche@...disk.com
Cc:     varun@...lsio.com, agrover@...hat.com, bgly@...ibm.com,
        target-devel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-scsi@...r.kernel.org, namei.unix@...il.com,
        stable@...r.kernel.org, Jianfei Hu <hujianfei@...s.chinamobile.com>
Subject: Re: [PATCH] target/user: Fix use-after-free cmd->se_cmd if the cmd
 isexpired


Hi Mike

Thanks very much for your analysis.

>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
>> index 2e33100..6396581 100644
>> --- a/drivers/target/target_core_user.c
>> +++ b/drivers/target/target_core_user.c
>> @@ -684,7 +684,6 @@ static int tcmu_check_expired_cmd(int id, void *p, void *data)
>>   
>>   	set_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags);
>>   	target_complete_cmd(cmd->se_cmd, SAM_STAT_CHECK_CONDITION);
>> -	cmd->se_cmd = NULL;
>>   
> How did tcmu_handle_completion get to a point it was accessing the
> se_cmd if the TCMU_CMD_BIT_EXPIRED bit was set?
> Were memory accesses out
> of order?
No, even using the -O3, becuase has there memory dependency ?

> CPU1 set the TCMU_CMD_BIT_EXPIRED bit then cleared
> cmd->se_cmd, but CPU2 copied cmd->se_cmd to se_cmd and saw it was NULL
> but did not yet see the TCMU_CMD_BIT_EXPIRED bit set?

Because the debug rpms for my kernel version were lost, and the crash
tools couldn't be used to have a more accurate analysis.
>
> It looks like, if you do the above patch, the above function will call
> target_complete_cmd and tcmu_handle_completion will call it again, so we
> will have a double free issue.
Maybe the best resolution is to move tcmu_handle_completion() between
spin_lock(&udev->commands_lock) and spin_unlock(&udev->commands_lock)?

Thanks.

BRs
Xiubo Li