Message-ID: <ea695f68-c90a-e471-bc24-1c572d54878d@oracle.com>
Date:   Thu, 2 Mar 2023 10:38:08 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH-next] scsi: fix use-after-free problem in
 scsi_remove_target
From:   Mike Christie <michael.christie@oracle.com>
To:     zhongjinghua <zhongjinghua@huaweicloud.com>,
        zhongjinghua <zhongjinghua@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yukuai3@huawei.com
References: <20230213034321.3261114-1-zhongjinghua@huaweicloud.com>
 <4585779e-f919-0439-2062-b1f30b04f176@huawei.com>
 <a71be678-7f40-811b-4612-81a4eeb910dd@huaweicloud.com>
 <c6106113-7050-4099-8c6b-ec79b6b83d5f@oracle.com>
Content-Language: en-US
In-Reply-To: <c6106113-7050-4099-8c6b-ec79b6b83d5f@oracle.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NXBhYldNTWxzNGpOeG1TVWdnT00zRXFzUEg3ekhWeVRXN3NGdlpZRkVOdlFI?=
 =?utf-8?B?Mkt6azhhRldkVTVhd09zZ0tndWhqY1ZCaUtESTAzbHdVY1oxSm1hVzMvajJ0?=
 =?utf-8?B?dE94TnlqOFh6c1d5QzBiWUpEcEtkQWRoakxiY2VVQlNkbG14NER5V3VpQ0FU?=
 =?utf-8?B?TTJrdWZROGVTREtTL3RJTDdDMkttc3hDRFZrQUtnUkQ2TGlkQzBJVVVic0dx?=
 =?utf-8?B?MjkzZ3R6MTRSSW0zWUFENTFSUEtrZU9yU0FnaXBzQnpsWkxGWWNnWVc0TjhQ?=
 =?utf-8?B?a0l3amFXaW9yUGtHZVVGaWtYUmlxdlNCNXV3T0JsbnJWTEhKK0R4RTFHS2JJ?=
 =?utf-8?B?WEJUOTJROUJyN01YSjVZeEkzMkFOeWZZR1NsNjNaMnZ6TThTbTNRWUY3ME1T?=
 =?utf-8?B?MVZDS1hLMVYzbEJYYkdlaFZBTFVQeU0xdGZtV3N0TzBMNUQ5M1hOMGR0TXha?=
 =?utf-8?B?dDFSUUlCc2M1Vk53MkpwVXdWcmh5anJnU3JIUVE1K2JYQWkrak56N2dyTm5Z?=
 =?utf-8?B?SENWOWQxNnVjMEVWekR3alNmdFJzc2tjMWt3LzZqcmJnRXZIelR2OGRiTytH?=
 =?utf-8?B?V1lzWGtqN1hLZExzTzJrT2lVMmlaT3c0QTRuVy8zc2xKUjFoSEQ4RktOSktG?=
 =?utf-8?B?TXlGckZmbVB3WjhqUzRsSXM2dFVHYm0ySHJ6TS9xcHpSaGVKRHVKSVUwaHY0?=
 =?utf-8?B?TkNsYWFOVU9SNzlGWndMQXBxVzJmUTZmVEFYc01ycUtHOWFHa09PTmJHMWlB?=
 =?utf-8?B?LzNYaUlOVUpzbzRpVnZoNXpXUFdIWTRTTnhKbFdJbGdyS1p3c0piV24vamgr?=
 =?utf-8?B?WU9MVU5LTGtVTWkzamIzempFUTlOTVVjRWlQMmhaWCt1b1FYRCt3T3dyeDI0?=
 =?utf-8?B?V0Q5SE11cis1T1Q0dFp3bGxJVVlINGNZMFRjUnpnR2l2WjZKUGRVVVFrblN5?=
 =?utf-8?B?NEJEdE1DLytPTmxFZEtSUWJzczlYYnY2bG5ZampEbDNFeTdqN0o4OVo4UEYz?=
 =?utf-8?B?SSt0NXJlVllTYkNoVkR2Y21zNjNNbk1pTjR0VG5jdHRTQlJ4ckxYdS9JZlFD?=
 =?utf-8?B?Tk9lQXA4d1pqRmFjdllWTEdlanNRSG9IZXdxTStZcFBkdFZreGFPRis0dzZa?=
 =?utf-8?B?d0trZTJTckpBZktpMWYrazRIU2g1SGtoaWt6SVY4ZWFGTTAzazIrcXlaWlBK?=
 =?utf-8?B?MFBmaWRDWTZvY09talYraFl3M1ROT3NhUTl0cGptSktXbnBXVm5sem1PS2pV?=
 =?utf-8?B?OVVyOHhYSGxhNGthbjdhODlsNWJmU29sMWttZTcwUFZGbktwa0JpcWlDK0gv?=
 =?utf-8?B?WDUxQytSc0FacjgwdkVySUNLVyttUFMxV2xQWHA4WjNNQzJtN0pDTXd3ZE5W?=
 =?utf-8?B?OWpQaG42TEVsV2FIUm5HTmQvZlFEY2h0S1FLUWw0cmpxTkJiM3J0bWhUMFhs?=
 =?utf-8?B?UDloQkZzVSttRHBxYmFjYUhwMUJwZFFzY1dVZVF1SUJoZGpxTVJVYjdYa2or?=
 =?utf-8?B?aEZxY1g2VWxkL2NrYVc2M1djUFBzZXZIMHExTys0WHJCWWs2K1dibFRCaHZy?=
 =?utf-8?B?OUlTRGdCZVpLb3E1ZWdKdjVCK2tXVmV0Y00xd0FEcEZtK0gvYmtBcG5USjJk?=
 =?utf-8?B?NmgyQm5UVW01bURZQzJQNHRna01ZZnVZenppYVJTSW5Jd2dicWF1aXZ6SkEw?=
 =?utf-8?B?WVdrcmlmQ2UvYTBSYk00SkV4ZEFrNE9XQStwTTR4M2N1d1VqQnFDUlJaa2cw?=
 =?utf-8?B?K2Z5QWRpUm9vWUNJVmtvNzRWZDVyWVRrTzViYys1eE02U05FRlBrRXZ0VG9w?=
 =?utf-8?B?QjZQaTRsTWxkMEFFWjJMZTczMC9obHRsLzN2VVVGY09TOVdwRTN5bFNRWjZh?=
 =?utf-8?B?NGdYVjh1UERlVjc3TXlIMEJYL0o2ai9OMFRwVFRXVDBpWXdFNkhCTFpXY3ll?=
 =?utf-8?B?czBKN2RwZTQyMXF4YTh2TGVGZXdoY3BZWUo2MXl6anF6enBlVlp0aTd3bFcr?=
 =?utf-8?B?ZEFHdkFpZVBNWTVBUkRsb05rVmh6bndqUW5qbGFlelJVUWIxSm05eDA2Rm9z?=
 =?utf-8?B?c3JSbDZCU2l6STVVeW9xa2t0RVpPY0tibmlDTEh1YVN1ektVZTFFZFRRSkgw?=
 =?utf-8?B?aW5UN2g3U0x3azNNU0JGUjVjcjM3aFVIL3BTMWJ1c2ZCeTVJNUFIckV1cmpK?=
 =?utf-8?B?VHc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: uDzOsCQ3wYlfT29Yr545gmxv6kEkzyxbgQRk7D6wke8Lm4TkyExHuMtdp+gwoc1p6fU3nX4zMVeMgKWENUD+31PolN5WhhciUE5K8Zixg0asGgdpcKysTCgXTpps5cXDzBUIL898XTqGOneYMIRCAwmjRjD1PmuHNOxOVCixKYdWFaAdvNalbTLngIhdSnKRE2U1Hr3RnLXq22Fy+evgvAzn9dXrvEq9Ok8t2Yo5VSWUJOgKcUEINHEwG9Et+GR8SbJfiglJdfAQ+ZdgPGo+galX1Q0Az0CMtcRGdqoJErv9riTSppqMEzv7P8vsxkFcXieceD/BORPAi68BPeH2wZBYvi0RpPPPJQMcJGP3xQ759oE6w4QLCOBAeYin6jEaGNzwktAUMIwAs22T1Rz3pMhUAjLpE0uRAXWWmjcdNliuLTDRBf4+TS1ZOLUta1gf430OOZR95gEiFkEWixIeJPm011qfYRtTDKoOJvWHUD9rcT9Qef15a5SSqf6fBZYTNExO0oBaqslP5XqxnaBR2QAWwDzKSn8C3ZfzF6w8kqqzzeTap+FpAp4A0K51vm8f4py/VbU5tvkgHvfgy6+KYS55+1sXA64xVw4ysLIUnAlACu4sZqRnR1bq5CDBdaz2enusZh51macDgcwdFDVZ37nkUqyqUF69reLrTnSlN2VpzTGbId6G5bzbAFfpnuKJi06/rpMl02N4kBOBgMoRS2Y/ObtDHxGEwPfx8GyD0dkBPbg3k8RB9VPOKCKrGj0eXzttUQxDmZGS5q9fx+4hJ7uw4k6jv+bIfa4tKt4a/lkWvuCIkK5FKXgViXN9vmSgl7WDYQLm/1p+oFNKr4lMeZXQCycawVmJtbNHRuHvnXmQnCt3V9pe2dAkzxJCO2+N
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 18813222-e163-427e-7979-08db1b3c822a
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2023 16:38:10.1522
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kUYJraaWFwj0z75FSv+PVv1z8EN0IzdxTwW8flHKXETJclGQPkV5hafnRTs1W3Vqo/UV78Wink3qVY9gDFsHSIuiYV6IdBTv2uQDY4mxQfI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5066
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-02_10,2023-03-02_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 bulkscore=0
 malwarescore=0 mlxlogscore=999 phishscore=0 spamscore=0 adultscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2303020144
X-Proofpoint-GUID: GJYQ1oqDo5QvGPJ_ZHYv9RYsx6pEoQsb
X-Proofpoint-ORIG-GUID: GJYQ1oqDo5QvGPJ_ZHYv9RYsx6pEoQsb
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/1/23 3:15 PM, Mike Christie wrote:
> On 2/28/23 9:40 PM, zhongjinghua wrote:
>>> 在 2023/2/13 11:43, Zhong Jinghua 写道:
>>>> From: Zhong Jinghua <zhongjinghua@huawei.com>
>>>>
>>>> A use-after-free problem like below:
>>>>
>>>> BUG: KASAN: use-after-free in scsi_target_reap+0x6c/0x70
>>>>
>>>> Workqueue: scsi_wq_1 __iscsi_unbind_session [scsi_transport_iscsi]
>>>> Call trace:
>>>>   dump_backtrace+0x0/0x320
>>>>   show_stack+0x24/0x30
>>>>   dump_stack+0xdc/0x128
>>>>   print_address_description+0x68/0x278
>>>>   kasan_report+0x1e4/0x308
>>>>   __asan_report_load4_noabort+0x30/0x40
>>>>   scsi_target_reap+0x6c/0x70
>>>>   scsi_remove_target+0x430/0x640
>>>>   __iscsi_unbind_session+0x164/0x268 [scsi_transport_iscsi]
>>>>   process_one_work+0x67c/0x1350
>>>>   worker_thread+0x370/0xf90
>>>>   kthread+0x2a4/0x320
>>>>   ret_from_fork+0x10/0x18
>>>>
>>>> The problem is caused by a concurrency scenario:
>>>>
>>>> T0: delete target
>>>> // echo 1 > /sys/devices/platform/host1/session1/target1:0:0/1:0:0:1/delete
>>>> T1: logout
>>>> // iscsiadm -m node --logout
>>>>
>>>> T0                            T1
>>>>   sdev_store_delete
>>>>    scsi_remove_device
>>>>     device_remove_file
>>>>      __scsi_remove_device
>>>>                              __iscsi_unbind_session
>>>>                               scsi_remove_target
>>>>                           spin_lock_irqsave
>>>>                                list_for_each_entry
>>>>       scsi_target_reap // starget->reaf 1 -> 0
>>>> kref_get(&starget->reap_ref);
>>>>                           // warn use-after-free.
>>>>                           spin_unlock_irqrestore
>>>>        scsi_target_reap_ref_release
>>>>     scsi_target_destroy
>>>>     ... // delete starget
>>>>                           scsi_target_reap
>>>>                           // UAF
>>>>
>>>> When T0 reduces the reference count to 0, but has not been released,
>>>> T1 can still enter list_for_each_entry, and then kref_get reports UAF.
>>>>
>>>> Fix it by using kref_get_unless_zero() to check for a reference count of
>>>> 0.
>>>>
>>>> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
>>>> ---
>>>>   drivers/scsi/scsi_sysfs.c | 12 +++++++++++-
>>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>>> index e7893835b99a..0ad357ff4c59 100644
>>>> --- a/drivers/scsi/scsi_sysfs.c
>>>> +++ b/drivers/scsi/scsi_sysfs.c
>>>> @@ -1561,7 +1561,17 @@ void scsi_remove_target(struct device *dev)
>>>>               starget->state == STARGET_CREATED_REMOVE)
>>>>               continue;
>>>>           if (starget->dev.parent == dev || &starget->dev == dev) {
>>>> -            kref_get(&starget->reap_ref);
>>>> +
>>>> +            /*
>>>> +             * If starget->reap_ref is reduced to 0, it means
>>>> +             * that other processes are releasing it and
>>>> +             * there is no need to delete it again
>>>> +             */
>>>> +            if (!kref_get_unless_zero(&starget->reap_ref)) {
>>>> +                spin_unlock_irqrestore(shost->host_lock, flags);
>>>> +                goto restart;
>>>> +            }
>>>> +
> 
> Patch looks ok.
> 
> Is there another bug in the existing kref_get_unless_zero(&starget->reap_ref)
> call in scsi_alloc_target?
> 
> I think scsi_alloc_target can find the target on the __targets list, and
> it's call to kref_get_unless_zero will succeed if we are only above getting
> our own ref (we have not done __scsi_remove_target and have not done the
> scsi_target_reap call at the end of the function).
> 
> But if scsi_remove_target has set the target state to STARGET_REMOVE, the thread
> that did scsi_alloc_target wouldn't be able to put the target into the correct state
> (the scsi_target_add call will see the target state and return). So later if the
> driver/transport class did scsi_remove_target again to remove the target that
> the scsi_alloc_target call re-added, we see the target->state still in STARGET_REMOVE
> and it won't get deleted.
> 
> Can we solve both issues at the same time?

I looked into this last part of my comment, and I don't think it's possible.
I thought we could just change around when we add/delete the target from the
__targets list and when the target_alloc/destroy callouts are done, but that
is more difficult than it looks.