Messages in this thread | | | Date | Fri, 14 Jul 2023 12:29:21 +0200 | Subject | Re: [PATCH 2/3] drm/scheduler: Fix UAF in drm_sched_fence_get_timeline_name | From | Christian König <> |
| |
Am 14.07.23 um 12:07 schrieb Asahi Lina: > On 14/07/2023 18.51, Christian König wrote: >> Am 14.07.23 um 11:44 schrieb Asahi Lina: >>> On 14/07/2023 17.43, Christian König wrote: >>>> Am 14.07.23 um 10:21 schrieb Asahi Lina: >>>>> A signaled scheduler fence can outlive its scheduler, since fences >>>>> are >>>>> independencly reference counted. Therefore, we can't reference the >>>>> scheduler in the get_timeline_name() implementation. >>>>> >>>>> Fixes oopses on `cat /sys/kernel/debug/dma_buf/bufinfo` when shared >>>>> dma-bufs reference fences from GPU schedulers that no longer exist. >>>>> >>>>> Signed-off-by: Asahi Lina <lina@asahilina.net> >>>>> --- >>>>> drivers/gpu/drm/scheduler/sched_entity.c | 7 ++++++- >>>>> drivers/gpu/drm/scheduler/sched_fence.c | 4 +++- >>>>> include/drm/gpu_scheduler.h | 5 +++++ >>>>> 3 files changed, 14 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c >>>>> b/drivers/gpu/drm/scheduler/sched_entity.c >>>>> index b2bbc8a68b30..17f35b0b005a 100644 >>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c >>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c >>>>> @@ -389,7 +389,12 @@ static bool >>>>> drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) >>>>> /* >>>>> * Fence is from the same scheduler, only need to wait >>>>> for >>>>> - * it to be scheduled >>>>> + * it to be scheduled. >>>>> + * >>>>> + * Note: s_fence->sched could have been freed and >>>>> reallocated >>>>> + * as another scheduler. This false positive case is okay, >>>>> as if >>>>> + * the old scheduler was freed all of its jobs must have >>>>> + * signaled their completion fences. >>>> >>>> This is outright nonsense. As long as an entity for a scheduler exists >>>> it is not allowed to free up this scheduler. >>>> >>>> So this function can't be called like this. >>>> >>>>> */ >>>>> fence = dma_fence_get(&s_fence->scheduled); >>>>> dma_fence_put(entity->dependency); >>>>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c >>>>> b/drivers/gpu/drm/scheduler/sched_fence.c >>>>> index ef120475e7c6..06a0eebcca10 100644 >>>>> --- a/drivers/gpu/drm/scheduler/sched_fence.c >>>>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c >>>>> @@ -68,7 +68,7 @@ static const char >>>>> *drm_sched_fence_get_driver_name(struct dma_fence *fence) >>>>> static const char *drm_sched_fence_get_timeline_name(struct >>>>> dma_fence *f) >>>>> { >>>>> struct drm_sched_fence *fence = to_drm_sched_fence(f); >>>>> - return (const char *)fence->sched->name; >>>>> + return (const char *)fence->sched_name; >>>>> } >>>>> static void drm_sched_fence_free_rcu(struct rcu_head *rcu) >>>>> @@ -216,6 +216,8 @@ void drm_sched_fence_init(struct drm_sched_fence >>>>> *fence, >>>>> unsigned seq; >>>>> fence->sched = entity->rq->sched; >>>>> + strlcpy(fence->sched_name, entity->rq->sched->name, >>>>> + sizeof(fence->sched_name)); >>>>> seq = atomic_inc_return(&entity->fence_seq); >>>>> dma_fence_init(&fence->scheduled, >>>>> &drm_sched_fence_ops_scheduled, >>>>> &fence->lock, entity->fence_context, seq); >>>>> diff --git a/include/drm/gpu_scheduler.h >>>>> b/include/drm/gpu_scheduler.h >>>>> index e95b4837e5a3..4fa9523bd47d 100644 >>>>> --- a/include/drm/gpu_scheduler.h >>>>> +++ b/include/drm/gpu_scheduler.h >>>>> @@ -305,6 +305,11 @@ struct drm_sched_fence { >>>>> * @lock: the lock used by the scheduled and the finished >>>>> fences. >>>>> */ >>>>> spinlock_t lock; >>>>> + /** >>>>> + * @sched_name: the name of the scheduler that owns this >>>>> fence. We >>>>> + * keep a copy here since fences can outlive their scheduler. >>>>> + */ >>>>> + char sched_name[16]; >>>> >>>> This just mitigates the problem, but doesn't fix it. >>> >>> Could you point out any remaining issues so we can fix them? Right now >>> this absolutely *is* broken and this fixes the breakage I observed. If >>> there are other bugs remaining, I'd like to know what they are so I >>> can fix them. >>> >>>> The real issue is that the hw fence is kept around much longer than >>>> that. >>> >>> As far as I know, the whole point of scheduler fences is to decouple >>> the hw fences from the consumers. >> >> Well yes and no. The decoupling is for the signaling, it's not >> decoupling the lifetime. > > When I spoke with Daniel I understood the intent was also to decouple > the lifetime.
Yes, we discussed that a long long time ago as well.
We *wanted* to decouple the dma_fence lifetime, it's just not done that way because of problems with that approach.
> >>> I already talked with Daniel about this. The current behavior is >>> broken. These fences can live forever. It is broken to require that >>> they outlive the driver that produced them. >>> >>>> Additional to that I'm not willing to increase the scheduler fence >>>> size >>>> like that just to decouple them from the scheduler. >>> >>> Did you read my explanation on the cover letter as to how this is just >>> broken right now? We need to fix this. If you have a better suggestion >>> I'll take it. Doing nothing is not an option. >> >> Well this isn't broken at all. This works exactly like intended, you >> just want to use it for something it wasn't made for. >> >> That scheduler fences could be changed to outlive the scheduler which >> issued them is possible, but this is certainly a new requirement. >> >> Especially since we need to grab additional references to make sure that >> the module isn't unloaded in such a case. > > Yes, that's a remaining issue. The fences need to grab a module > reference to make sure drm_sched doesn't get unloaded until they're > all really gone. I can add that in v2.
You also need to come up with an idea to prevent races with the deadline handling. See drm_sched_fence_set_deadline_finished().
> > It would also be desirable to drop the hw fence as soon as it signals, > instead of keeping a reference to it forever.
Yeah, agree. Problem here again is that this is easier said than done in a non-racy way.
Christian.
> > ~~ Lina >
| |