lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 26 Nov 2020 09:13:53 +0100
From:   Cornelia Huck <cohuck@...hat.com>
To:     Qinglang Miao <miaoqinglang@...wei.com>
Cc:     Benjamin Block <bblock@...ux.ibm.com>,
        Steffen Maier <maier@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        <linux-s390@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

On Thu, 26 Nov 2020 09:27:41 +0800
Qinglang Miao <miaoqinglang@...wei.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@...wei.com>
> >> Signed-off-by: Qinglang Miao <miaoqinglang@...wei.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?

Powered by blists - more mailing lists