Message-ID: <c6106113-7050-4099-8c6b-ec79b6b83d5f@oracle.com>
Date:   Wed, 1 Mar 2023 15:15:03 -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
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>
Content-Language: en-US
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <a71be678-7f40-811b-4612-81a4eeb910dd@huaweicloud.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?ZXV1OWZaR01yYUMrMTNSSG5BdHNXOGlTTm5JNUp3OXU1UzI0bmtOT0hIYjll?=
 =?utf-8?B?alJ3VnVTTE9WemJKNklHdUVNQThSVXZCL0Q0ZVFUT1U2Q1hQaUdub2gwSkdp?=
 =?utf-8?B?bUNtL3ZRdmpyR00xVTJCREpsallhSmZuV255akdWbGhWZ3ZsOXgxNHFTbkRm?=
 =?utf-8?B?aHcvU0Njc29adWR2Q0lqN2xIbXJDSlJyQm1KbWt6UkVlMnBtWm05OHpobTU3?=
 =?utf-8?B?Mkl4T1Z6VXUyeHdOMFZlTTlnSjRQUVlsZ3pPTE1Wa0lTOU1jaENOUVhONzhI?=
 =?utf-8?B?cjQ1cU9ZWmpkRm44WHBMSkVUbmdwdmQ1ZnBNSEZyY3dmdzlVNVk2R1MrUStY?=
 =?utf-8?B?OGhCK1BlVWJ1MVd6c05vamtaVjBHNmNXNEdyUStOZ05QK2svWmV2aFRkOGp1?=
 =?utf-8?B?TkRvblJqc0pzL2E2RzNGUzhCWGRmQnlqWW1MVFNsK0VtZG0wNjFFeTNrWTdv?=
 =?utf-8?B?U0pmdWFJYWNxbG44RW5jZlRzaEhBb1Z0SjROdDhoTzlDN2s0NjNLcU5tYmFy?=
 =?utf-8?B?OFYrT2MzdXZ4UnFOOHhCaGFFbWd2LzVhMUpibE1IN1RLQW1LOFRDRzdXQ0k0?=
 =?utf-8?B?ZjI3dGlyVEhjVnExVjRlTlRRRUVmMU9CR0haUnRwalZpVEhpV2dWZFNlMVZx?=
 =?utf-8?B?S01tdVE0YTU4cGJ5Nk9lc1d2WDBzVW5tSm5hK0R6NHlEWVZlYlI4NEZ5Wi9m?=
 =?utf-8?B?dE5OYjUwcERVdnVBS3RxQUVVNlRIc0hOWTBGYjF5c29BbGZRZ2tNSFk4UDVI?=
 =?utf-8?B?UjhTc1JwVlh2d05lK1Z5dTVYQytYVDNiQXlCVDhhOUJ2QVFKTDdHU3liS2xF?=
 =?utf-8?B?YlVCaUxvdmJzM2RudE15RjRRTmU1dW96bzMzMTJLNlFoQ0I1bzV3RWZ4Y292?=
 =?utf-8?B?VUI5WTVLNUhCMUI0T2RZS2FtNEpQV0Q5VEx2S3VWWFFEWEdlVTRsRVQ4aTZt?=
 =?utf-8?B?N3V3SkxCWTRmVWMva215S2l1bFJzUFJIdWpRZjlLVU52T2lCT0U4QzJRRnZt?=
 =?utf-8?B?eWxTK2hiS29wYS9yWURiSVM3OUJ1OHNZcDE5c1hZcnA2WGlWL3FsT3kvUDRM?=
 =?utf-8?B?aEtlTnptdEJsYnk3MGZuWTJ1aklTMUJuK3ltemtjYzdicVFGb2xzTFFCKzNH?=
 =?utf-8?B?NE5VV1VPNFRIeXdCRmxaeEZVay91dnlqZE1JSVN6YkVLOExzV3poYk1UZ3Bq?=
 =?utf-8?B?RUI3RGF0RkJTMDBnQ05qbCtpVjlIc29JYjdBd1ZROXl4dTlFNVhGb01lR3lm?=
 =?utf-8?B?MnVtTFNWaEtzVmtMNlZtR0tRdnNySWc0NGJ4MGViWVB2RSt4bTNoZkhqbWU0?=
 =?utf-8?B?R0s4MXo1Yk5OK2VaQXBFdHpTMGo5UTJBTFlKK0p3YXpDQ0FOdWpXRnQ5MWhD?=
 =?utf-8?B?SyttTGV0Z2lCUWJQb2YzVElQenFpY095N2NtbWJPUEsvMmdKOWpkMFRHTmNq?=
 =?utf-8?B?ekJtZGJXQ0l0dU4vRVZONHZXRlJPUTQ0QkxGK2wxd2k1elJJZFZzekFFc3cr?=
 =?utf-8?B?OFMrNWx4eHBob2JTRlpYL0VBSjlDWlJkK2FlQ1hBenl6MjlTdGsrUHQzeC9Z?=
 =?utf-8?B?dkM1MW9yanpYT0tNb1c5Z1NUVGlxZ1FNM1RNb0RvOWhvWHJHL1k0TjY5UCtk?=
 =?utf-8?B?WWsxT3VZSkFuS3E4Mm9qWit3RW84WWlVUDJva0I5ZlBFcXNraUYrTldIaENu?=
 =?utf-8?B?bmRtcG92L1l3QUtOVFU5NG5DUnRBbkVnQnhDYU5sU0oxbDd2V3ptcFRkb0RX?=
 =?utf-8?B?VUtTRHl6YTRod1hXL1cyVjhXaEcwNDZrRWZiM1VnYkZUejdxTHNJZVJBZVlS?=
 =?utf-8?B?eEg5c2VQQ2Q2L0VGM3ZYMXVCaVY2YW9SdVFBclo4UjNHS2lxZFpCbjNWWllz?=
 =?utf-8?B?VWZQTEEwQkRXTzJBRWhTVUtPMzZPV0xOK0xqUmw1REMwME5TWU44MURGSzFj?=
 =?utf-8?B?Zk1nM2l5VXdHN2JTQkpZaDBJRUJNNEZMNWdUbTRRdVR5V0s4YkdVbklZbmU3?=
 =?utf-8?B?L0VmcC9tNWRIMFdSOHIxZEdiajM5eG9CS1hyYWpTa3BOeU1kV29aNHh5aWo2?=
 =?utf-8?B?bFA5NG9XMmx3ZWlvN2lCZkRYd1hJZjVkdkZqbklIZ2M5UGJPMEljKzluemVZ?=
 =?utf-8?B?T01mQnNnUTR5NjIzZ3RBWjRFMjZ3Z2NNU1RLbEtzdm5lVVpXY0hNbVlmY3M1?=
 =?utf-8?B?Nnc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: vzPA3WaK5TR3MCQJiZ0frqEvmEIZ1q/yxMcbw+Yct+Ue0neP+DU4jnADIUkWTNZ4OOyz1Y1jQwok3AcZgzQ04Y4BZZx9vTU9FAgHsDh1P/9233MtH3owV44SABY3/HAH5bLZE6ZZItWB813bKbN1gKq7vaeQM1rsvJAIqj0v4IruEz+ZRPNdMQWwRc+wfsYLRNYZ3s8jnZJUALAeiU/CcnCXOSu97CMWXwc4XPej1GoMVkZamnrrLWZrC12MUzzpCq/w4JOBaI9TL6mcwbHeAI456O0HH094szetftsi/QrFWa8MkL4NkA+FbkrSzAc2dJ2IgOrBajVrhdhfqi85xVK8pN8HTiq3B4lQC4FJ4QIiPum2uh8jfoLZYRNa0ySwWPKkOqyOK+GyVIafp1HbUcJYketRjPo1hj9bq9leb76F13mkmLWmVbD4V+ym4FAdKsYLuuNgDCa4bC3YbKgeW6K9uBYlHwvAexEgYwcpCAe9LZdyZaWVpN+0oZtr6bnXZX+IQ0iVHOep8TxWCzkCJz+49/4XmlJ9XMRMScWi0OjegjAqZY7RGss9zSi7WWg0OwShtBRyaW4xogeaxXlH5ArICT1fC3k3EirXWGjVnk2y+OZiwpVMP4pKONIl7YNcnf+L1nNMMB18mO8nxrI8GBbpmip0dcLcBeG5trMSmrNxidodSs/PJj2Sq322cAbxaJVMsUUEMWwU5zMepx8SK7jiPAYNihcQHHG7PfcFppgXGXYbfJ1LLbW4EGy8Zd+M1iwMvOc5DTGq7izTNw936+LXkbENVmO35OD0sSXPH3UfQfePwz6lNhJn1zj7ZrK05HJSPNL5RMf3D+V5/h+U9bmeyKTqGNlTUBpOrTM12Aitug8Km3G/JeoYpNAhKbEe
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4dbdc7e2-2eca-446f-57ed-08db1a9a06d0
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Mar 2023 21:15:04.6535
 (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: l/uwk0ibl5+6c7r176bXN27GymLYK4SDFbSEVQEAULpLkggkOExA6HMTgo5+iC+4X180cLqBNPrib7EagZGuNOcEqQ2G3UPxzVO/qxicft8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4852
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-01_15,2023-03-01_03,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 adultscore=0
 mlxlogscore=999 suspectscore=0 phishscore=0 spamscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2303010168
X-Proofpoint-GUID: -NZx8-otEISGun4oVB1N2Ls3644EYr5M
X-Proofpoint-ORIG-GUID: -NZx8-otEISGun4oVB1N2Ls3644EYr5M
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?