[PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Qinglang Miao]

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;
-- 
2.23.0

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[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.

-- 
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

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Qinglang Miao]

在 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.

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Cornelia Huck]

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?

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[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.


-- 
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

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Qinglang Miao]

在 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()?
> 

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Benjamin Block]

On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote:
> 在 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:
....
> > 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()?

Feel free to do so.

I think having the `put_device()` call after `device_unregister()` in
both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more
natural, because it ought to be the last time we touch the object in
both functions.


-- 
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

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Steffen Maier]

On 11/26/20 4:12 PM, Benjamin Block wrote:
> On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote:
>> 在 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:
> ....
>>> 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()?
> 
> Feel free to do so.
> 
> I think having the `put_device()` call after `device_unregister()` in
> both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more
> natural, because it ought to be the last time we touch the object in
> both functions.

If you move put_device(), you could add a comment like we did here to explain 
which (hidden) get_device is undone:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=ef4021fe5fd77ced0323cede27979d80a56211ca
("scsi: zfcp: fix to prevent port_remove with pure auto scan LUNs (only sdevs)")
So in this patch it could be:
	put_device(&unit->dev); /* undo _zfcp_unit_find() */
And in the other patch it could be:
	put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */
Then it would be clearer next time somebody looks at the code.

Especially for the other patch on zfcp_sysfs_port_remove_store() moving the 
put_device(&port->dev) to at least *after* the call of 
zfcp_erp_port_shutdown(port, 0, "syprs_1") would make the code cleaner to me. 
Along the idead of passing the port to zfcp_erp_port_shutdown with the 
reference we got from zfcp_get_port_by_wwpn(). That said, the current code is 
of course still correct as we currently have the port ref of the earlier 
device_register so passing the port to zfcp_erp_port_shutdown() is safe.

If we wanted to make the gets and puts nicely nested, then we could move the 
puts to just before the device_unregister, but that's bike shedding:
	device_register()   --+
	get_device() --+      |
	put_device() --+      |
	device_unregister() --+

Benjamin's suggested move location works for me, too. After all, the kdoc of 
device_unregister explicitly mentions the possibility that other refs might 
continue to exist after device_unregister was called:
	device_register()   --+
	get_device() ---------|--+
	device_unregister() --+  |
	put_device() ------------+

-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Qinglang Miao]

在 2020/11/27 17:21, Steffen Maier 写道:
> On 11/26/20 4:12 PM, Benjamin Block wrote:
>> On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote:
>>> 在 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:
>> ....
>>>> 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()?
>>
>> Feel free to do so.
>>
>> I think having the `put_device()` call after `device_unregister()` in
>> both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more
>> natural, because it ought to be the last time we touch the object in
>> both functions.
> 
> If you move put_device(), you could add a comment like we did here to 
> explain which (hidden) get_device is undone:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=ef4021fe5fd77ced0323cede27979d80a56211ca 
> 
> ("scsi: zfcp: fix to prevent port_remove with pure auto scan LUNs (only 
> sdevs)")
> So in this patch it could be:
>      put_device(&unit->dev); /* undo _zfcp_unit_find() */
> And in the other patch it could be:
>      put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */
> Then it would be clearer next time somebody looks at the code.
>
Hi, Steffen

Sorry I didn't notice this mail when I sent a patch to move put_device, 
you suggestion sounds resonable to me, so I send a v2 to add comments.

Thanks.
> Especially for the other patch on zfcp_sysfs_port_remove_store() moving 
> the put_device(&port->dev) to at least *after* the call of 
> zfcp_erp_port_shutdown(port, 0, "syprs_1") would make the code cleaner 
> to me. Along the idead of passing the port to zfcp_erp_port_shutdown 
> with the reference we got from zfcp_get_port_by_wwpn(). That said, the 
> current code is of course still correct as we currently have the port 
> ref of the earlier device_register so passing the port to 
> zfcp_erp_port_shutdown() is safe.
> 
> If we wanted to make the gets and puts nicely nested, then we could move 
> the puts to just before the device_unregister, but that's bike shedding:
>      device_register()   --+
>      get_device() --+      |
>      put_device() --+      |
>      device_unregister() --+
> 
> Benjamin's suggested move location works for me, too. After all, the 
> kdoc of device_unregister explicitly mentions the possibility that other 
> refs might continue to exist after device_unregister was called:
>      device_register()   --+
>      get_device() ---------|--+
>      device_unregister() --+  |
>      put_device() ------------+
Glad to know your opinions, I'd like to take this one on my patch.
>