linux-kernel - Re: [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8Dp9dM1MxhzuvmR@pollux>
Date: Thu, 27 Feb 2025 23:40:53 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Boqun Feng <boqun.feng@...il.com>,
	Joel Fernandes <joelagnelf@...dia.com>,
	Alexandre Courbot <acourbot@...dia.com>,
	Dave Airlie <airlied@...il.com>, Gary Guo <gary@...yguo.net>,
	Joel Fernandes <joel@...lfernandes.org>,
	John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
	paulmck@...nel.org
Subject: Re: [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice
 implementation

On Thu, Feb 27, 2025 at 06:00:13PM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 27, 2025 at 01:25:10PM -0800, Boqun Feng wrote:
> > 
> > Most of the cases, it should be naturally achieved, because you already
> > bind the objects into your module or driver, otherwise they would be
> > already cancelled and freed. 
> 
> I'm getting the feeling you can probably naturally achieve the
> required destructors, but I think Danillo is concerned that since it
> isn't *mandatory* it isn't safe/sound.

Of course you can "naturally" achieve the required destructors, I even explained
that in [1]. :-)

And yes, for *device resources* it is unsound if we do not ensure that the
device resource is actually dropped at device unbind.

Maybe some example code does help. Look at this example where we assume that
pci::Device::iomap_region() does return a pci::Bar instead of a
Devres<pci::Bar>, which it actually does. (The example won't compile, since,
for readability, it's heavily simplified.)

    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
        let bar: pci::Bar = pdev.iomap_region()?;

        let drvdata = Arc::new(bar, GFP_KERNEL)?;

        let drm = drm::device::Device::new(pdev.as_ref(), drvdata)?;
        let reg = drm::drv::Registration::new(drm)?;

        // Everything in `Self` is dropped on remove(), hence the DRM driver is
        // unregistered on remove().
        Ok(KBox::new(Self(reg), GFP_KERNEL)?)
    }

This is already broken, because now the lifetime of the pci::Bar is bound to the
DRM device and the DRM device is allowed to outlive remove().

Subsequently, pci_iounmap() and pci_release_region() are not called in remove(),
but whenever the DRM device is dropped.

The fact that this is possible with safe code, makes this API unsound.

Now let's assume iomap_region() would return a Devres<pci::Device>. That fixes
the problem, because even if the DRM device keeps the Devres<pci::Device> object
alive, it is still dropped when the driver is unbound, and subsequently
pci_iounmap() and pci_release_region() are called when they're supposed to be
called.

Note that this would not be a problem if pci::Device would not be a device
resource. Other stuff may be perfectly fine to bind to the lifetime of the DRM
device.

[1] https://lore.kernel.org/rust-for-linux/Z8CX__mIlFUFEkIh@cassiopeiae/