lkml.org 
[lkml]   [2020]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove
From
Date


在 2020/11/26 17:42, Benjamin Block 写道:
> On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote:
>> On Thu, 26 Nov 2020 09:27:41 +0800
>> Qinglang Miao <miaoqinglang@huawei.com> wrote:
>>
>>> 在 2020/11/26 1:06, Benjamin Block 写道:
>>>> On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote:
>>>>> kfree(port) is called in put_device(&port->dev) so that following
>>>>> use would cause use-after-free bug.
>>>>>
>>>>> The former put_device is redundant for device_unregister contains
>>>>> put_device already. So just remove it to fix this.
>>>>>
>>>>> Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage")
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
>>>>> ---
>>>>> drivers/s390/scsi/zfcp_unit.c | 2 --
>>>>> 1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c
>>>>> index e67bf7388..664b77853 100644
>>>>> --- a/drivers/s390/scsi/zfcp_unit.c
>>>>> +++ b/drivers/s390/scsi/zfcp_unit.c
>>>>> @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun)
>>>>> scsi_device_put(sdev);
>>>>> }
>>>>>
>>>>> - put_device(&unit->dev);
>>>>> -
>>>>> device_unregister(&unit->dev);
>>>>> >> return 0;
>>>>
>>>> Same as in the other mail for `zfcp_sysfs_port_remove_store()`. We
>>>> explicitly get a new ref in `_zfcp_unit_find()`, so we also need to put
>>>> that away again.
>>>>
>>> Sorry, Benjamin, I don't think so, because device_unregister calls
>>> put_device inside.
>>>
>>> It seem's that another put_device before or after device_unregister is
>>> useless and even might cause an use-after-free.
>>
>> The issue here (and in the other patches that I had commented on) is
>> that the references have different origins. device_register() acquires
>> a reference, and that reference is given up when you call
>> device_unregister(). However, the code here grabs an extra reference,
>> and it of course has to give it up again when it no longer needs it.
>>
>> This is something that is not that easy to spot by an automated check,
>> I guess?
>>
>
> Indeed.
>
> I do think the two patches for zfcp have merit, but not by simply
> removing the put_device(), but by moving it.
>
> For this patch in particular, I'd think the "proper logic" would be to
> move the `put_device()` to after the `device_unregister()`:
>
> device_unregister(&unit->dev);
> put_device(&unit->dev);
>
> return 0;
>
> As Cornelia pointed out, the extra `get_device()` we do in
> `_zfcp_unit_find()` needs to be reversed, otherwise we have a dangling
> reference and probably some sort of memory-/resource-leak.
>
> Let's go by example. If we assume the reference count of `unit->dev` is
> R, and the function starts with R = 1 (otherwise the deivce would've
> been freed already), we get:
>
> int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun)
> {
> struct zfcp_unit *unit;
> struct scsi_device *sdev;
>
> write_lock_irq(&port->unit_list_lock);
> // unit->dev (R = 1)
> unit = _zfcp_unit_find(port, fcp_lun);
> // get_device(&unit->dev)
> // unit->dev (R = 2)
> if (unit)
> list_del(&unit->list);
> write_unlock_irq(&port->unit_list_lock);
>
> if (!unit)
> return -EINVAL;
>
> sdev = zfcp_unit_sdev(unit);
> if (sdev) {
> scsi_remove_device(sdev);
> scsi_device_put(sdev);
> }
>
> // unit->dev (R = 2)
> put_device(&unit->dev);
> // unit->dev (R = 1)
> device_unregister(&unit->dev);
> // unit->dev (R = 0)
>
> return 0;
> }
>
> If we now apply this patch, we'd end up with R = 1 after
> `device_unregister()`, and the device would not be properly removed.
>
> If you still think that's wrong, then you'll need to better explain why.
>
Hi Banjamin and Cornelia,

Your replies make me reliaze that I've been holding a mistake
understanding of put_device() as well as reference count.

Thanks for you two's patient explanation !!

BTW, should I send a v2 on these two patches to move the position of
put_device()?
>

\
 
 \ /
  Last update: 2020-11-26 13:08    [W:0.088 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site