Messages in this thread | | | Subject | Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove | From | Qinglang Miao <> | Date | Thu, 26 Nov 2020 20:07:32 +0800 |
| |
在 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()? >
| |