Message-ID: <ZC6Vh2gG91zXUKJW@phenom.ffwll.local>
Date:   Thu, 6 Apr 2023 11:48:55 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Asahi Lina <lina@...hilina.net>
Cc:     Christian König <christian.koenig@....com>,
        Luben Tuikov <luben.tuikov@....com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH] drm/scheduler: Fix UAF in
 drm_sched_fence_get_timeline_name

On Thu, Apr 06, 2023 at 06:27:27PM +0900, Asahi Lina wrote:
> On 06/04/2023 18.15, Asahi Lina wrote:
> > On 06/04/2023 18.06, Christian König wrote:
> > > Am 06.04.23 um 10:49 schrieb Asahi Lina:
> > > > On 06/04/2023 17.29, Christian König wrote:
> > > > > Am 05.04.23 um 18:34 schrieb Asahi Lina:
> > > > > > A signaled scheduler fence can outlive its scheduler, since fences are
> > > > > > independently reference counted.
> > > > > 
> > > > > Well that is actually not correct. Schedulers are supposed to stay
> > > > > around until the hw they have been driving is no longer present.
> > > > 
> > > > But the fences can outlive that. You can GPU render into an imported
> > > > buffer, which attaches a fence to it. Then the GPU goes away but the
> > > > fence is still attached to the buffer. Then you oops when you cat that
> > > > debugfs file...
> > > 
> > > No, exactly that's the point you wouldn't ops.
> > > 
> > > > 
> > > > My use case does this way more often (since schedulers are tied to
> > > > UAPI objects), which is how I found this, but as far as I can tell
> > > > this is already broken for all drivers on unplug/unbind/anything else
> > > > that would destroy the schedulers with fences potentially referenced
> > > > on separate scanout devices or at any other DMA-BUF consumer.
> > > 
> > > Even if a GPU is hot plugged the data structures for it should only go
> > > away with the last reference, since the scheduler fence is referencing
> > > the hw fence and the hw fence in turn is referencing the driver this
> > > shouldn't happen.
> > 
> > So those fences can potentially keep half the driver data structures
> > alive just for existing in a signaled state? That doesn't seem sensible
> > (and it definitely doesn't work for our use case where schedulers can be
> > created and destroyed freely, it could lead to way too much junk
> > sticking around in memory).

This is why the split betwen the hw fence and the public sched fence.
Because once the hw fence stuff is sorted it should all be able to go
away, without the public fence keeping everything alive.

> > > > > E.g. the reference was scheduler_fence->hw_fence->driver->scheduler.
> > > > 
> > > > It's up to drivers not to mess that up, since the HW fence has the
> > > > same requirements that it can outlive other driver objects, just like
> > > > any other fence. That's not something the scheduler has to be
> > > > concerned with, it's a driver correctness issue.
> > > > 
> > > > Of course, in C you have to get it right yourself, while with correct
> > > > Rust abstractions will cause your code to fail to compile if you do it
> > > > wrong ^^
> > > > 
> > > > In my particular case, the hw_fence is a very dumb object that has no
> > > > references to anything, only an ID and a pending op count. Jobs hold
> > > > references to it and decrement it until it signals, not the other way
> > > > around. So that object can live forever regardless of whether the rest
> > > > of the device is gone.
> > > 
> > > That is then certainly a bug. This won't work that way, and the timelime
> > > name is just the tip of the iceberg here.
> > > 
> > > The fence reference count needs to keep both the scheduler and driver
> > > alive. Otherwise you could for example unload the module and immediately
> > > ops because your fence_ops go away.
> > 
> > Yes, I realized the module issue after writing the email. But that's the
> > *only* thing it needs to hold a reference to! Which is much more
> > sensible than keeping a huge chunk of UAPI-adjacent data structures
> > alive for a signaled fence that for all we know might stick around
> > forever attached to some buffer.
> > 
> > > > > Your use case is now completely different to that and this won't work
> > > > > any more.
> > > > > 
> > > > > This here might just be the first case where that breaks.
> > > > 
> > > > This bug already exists, it's just a lot rarer for existing use
> > > > cases... but either way Xe is doing the same thing I am, so I'm not
> > > > the only one here either.
> > > 
> > > No it doesn't. You just have implemented the references differently than
> > > they are supposed to be.
> > > 
> > > Fixing this one occasion here would mitigate that immediate ops, but
> > > doesn't fix the fundamental problem.
> > 
> > Honestly, at this point I'm starting to doubt whether we want to use
> > drm_scheduler at all, because it clearly wasn't designed for our use
> > case and every time I try to fix something your answer has been "you're
> > using it wrong", and at the same time the practically nonexistent
> > documentation makes it impossible to know how it was actually designed
> > to be used.
> 
> Also, requiring that the hw_fence keep the scheduler alive (which is
> documented nowhere) is a completely ridiculous lifetime requirement and
> layering violation across multiple subsystems. I have no idea how I'd make
> Rust abstractions uphold that requirement safely without doing something
> silly like having abstraction-specific hw_fence wrappers, and then you run
> into deadlocks due to the scheduler potentially being dropped from the
> job_done callback when the fence reference is dropped and just... no,
> please. This is terrible. If you don't want me to fix it we'll have to find
> another way because I can't work with this.

So generally the hw fence keeps the underlying gpu ctx alive, and the gpu
context keeps the gpu vm alive. Pretty much has to, or your gpu starts
executing stuff that's freed.

Now for fw scheduler gpu ctx isn't just drm_sched_entity, but also
drm_scheduler. Plus whatever driver stuff you have lying around for a ctx.
Plus ofc a reference to the vm, which might in turn keep a ton of gem_bo
alive.

Still I'm not seeing where the fundamental issue is in this refcount
scheme, or where it's silly? Mapping this all to Rust correctly is a
challenge for sure, and also untangling the assumption that drm_scheduler
is suddenly a lot more dynamic object (see my other reply), but
fundamentally calling this all silly and terrible I don't understand ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists