Re: Manual unbind of ATA devices causes use-after-free

[Tejun Heo]

Hello,

On Fri, Nov 03, 2017 at 09:32:16AM -0700, Taras Kondratiuk wrote:
> Quoting Tejun Heo (2017-11-03 06:19:37)
> > Hello,
> > 
> > On Wed, Nov 01, 2017 at 04:24:47PM -0700, Taras Kondratiuk wrote:
> > > Manual unbind/remove unconditionally invokes devres_release_all which
> > > calls ata_host_release() and frees ata_host/ata_port memory while it is
> > > still being referenced (e.g as a parent of SCSI host).
> > > 
> > > Is there a reason why ata_host is using derves which is not refcounted?
> > > Does it make sense to add recounting to ata_host?
> > 
> > Hmm... the removal path is supposed to drain everything synchronously.
> > What kind of controller is it?
> 
> It drains synchronously if scsi_host_put(ap->scsi_host) in
> ata_host_release() releases the last scsi_host reference. But when the issue
> happens there is one more reference to scsi_host because sg device is
> still open. The last reference will be dropped from sg_release.

I see.

> I forgot to mention that the disk may not be clearly unmounted when I'm
> unbinding it, but IMO it shouldn't cause use-after-free in the kernel.

Oh, it shouldn't.  It's a bug.

> Also even if sg_release() is called before ata_host_release() there is
> still no guarantee that the last reference will be dropped, because
> sg_release() schedules sg_remove_sfp_usercontext() to do actual release
> and the work may not be completed in time.

Hmmm, we didn't use to put in ata device structs in the kobject tree,
so this wasn't an issue.  This was changed by 9a6d6a2ddabb ("ata: make
ata port as parent device of scsi host") while adding the transport
support.  While doing that, we didn't change the release path to match
it, so the failure.

cc'ing Lin.  Lin, can you take a look at this?

Thanks.

-- 
tejun