Hi Krzysztof, On 2025-03-19 at 14:01:17 GMT, Krzysztof Karas wrote: > Some references to a perf stream in i915_oa_init_reg_state() > might remain active after its destruction in > i915_perf_release(). This could cause a read after free > condition as seen in issue #13756. > > Since i915_oa_init_reg_state() code already checks if stream > exists, set its reference (file->private_data) to NULL > explicitly. > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13756 > Signed-off-by: Krzysztof Karas <krzysztof.karas@xxxxxxxxx> > --- > I was not able to reproduce this issue locally, but got a note > from Chris Wilson offline that the problem might still exist, > so here is my attempt to remedy that. > > I am also unsure if adding "Fixes" tag for commit eec688e1420d > ("drm/i915: Add i915 perf infrastructure") here along with tag > for stable would be appropriate. > > I think invalidating the pointer to perf stream explicitly would > prevent issues with use-after-free in the future, but I'd like > to see what people think first, hence RFC. > That pointer is kfreed inside i915_perf_destroy_locked(), so I don't see an issue with making it NULL on top of that, especially if we don't know what code and when will access it. There are other instances in i915 where such a pattern emerges, so I doubt it would be a problem here. Just a cleaner deallocation. I'd also maybe investigate why that problematic stream gets released in the first place, and why we want to read from it after it has been released. Maybe there's a logic bug somewhere too; since if we end up calling .release we don't expect anything to be using the to-be-released structures anymore. > drivers/gpu/drm/i915/i915_perf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index bec164e884ae..ea1771da3f67 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -3743,6 +3743,9 @@ static int i915_perf_release(struct inode *inode, struct file *file) > */ > mutex_lock(>->perf.lock); > i915_perf_destroy_locked(stream); > + > + /* Make sure that any remaining references to this stream are invalid. */ > + file->private_data = NULL; > mutex_unlock(>->perf.lock); > > /* Release the reference the perf stream kept on the driver. */ Thanks Krzysztof
