linux-kernel - Re: [PATCH] driver core: Make it safe to use get

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 linux-cve-announce 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]

Message-ID: <20171214074227.GA30120@kroah.com>
Date:   Thu, 14 Dec 2017 08:42:27 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Jason Yan <yanaijie@...wei.com>
Cc:     martin.petersen@...cle.com, jejb@...ux.vnet.ibm.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bart Van Assche <bart.vanassche@....com>,
        "Ewan D . Milne" <emilne@...hat.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH] driver core: Make it safe to use get_device() if the
 reference count is zero

On Thu, Dec 14, 2017 at 11:39:36AM +0800, Jason Yan wrote:
> Some driviers may have the chance to increase a reference count that
> has dropped to zero when using get_device() because of their design.

Then those drivers are broken :)

> We have met such a issue with scsi:
> https://www.spinics.net/lists/linux-scsi/msg115295.html
> 
> The scsi core will keep the scsi device object in the host list after
> it has been deleted and the iterator can still find it. All of the
> places where need iterating have to check the state of the scsi device
> and this makes a lot of code redundancy and complexity.
> 
> Provide a safe mechanism in get_device() by using
> kobject_get_unless_zero().
> 
> Suggested-by: Bart Van Assche <bart.vanassche@....com>
> Signed-off-by: Jason Yan <yanaijie@...wei.com>
> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> CC: Bart Van Assche <bart.vanassche@....com>
> CC: Ewan D. Milne <emilne@...hat.com>
> CC: James E.J. Bottomley <jejb@...ux.vnet.ibm.com>
> CC: Christoph Hellwig <hch@....de>
> ---
>  drivers/base/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 12ebd05..cc74810 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1916,7 +1916,7 @@ EXPORT_SYMBOL_GPL(device_register);
>   */
>  struct device *get_device(struct device *dev)
>  {
> -	return dev ? kobj_to_dev(kobject_get(&dev->kobj)) : NULL;
> +	return dev && kobject_get_unless_zero(&dev->kobj) ? dev : NULL;

I really don't want to do this, the bus the device is on should prevent
this from happening.

Also, once that reference count drops to zero, the memory should be
freed, so you really have a stale pointer here, and this code would fail
if you had slab debugging enabled anyway.

So I don't even think this fixes the issue you think it fixes :)

thanks,

greg k-h