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


    --
    Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems
    IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
    Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp
    Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

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