| Message ID | 20230630021906.1035115-1-islituo@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2e63972a2de14482d0eae1a03a73e379f1c3f44c |
| Headers | show |
| Series | drm/exynos: fix a possible null-pointer dereference due to data race in exynos_drm_crtc_atomic_disable() | expand |
On 30/06/2023 04:19, Tuo Li wrote: > The variable crtc->state->event is often protected by the lock > crtc->dev->event_lock when is accessed. However, it is accessed as a > condition of an if statement in exynos_drm_crtc_atomic_disable() without > holding the lock: > > if (crtc->state->event && !crtc->state->active) > > However, if crtc->state->event is changed to NULL by another thread right > after the conditions of the if statement is checked to be true, a > null-pointer dereference can occur in drm_crtc_send_vblank_event(): > > e->pipe = pipe; > > To fix this possible null-pointer dereference caused by data race, the > spin lock coverage is extended to protect the if statement as well as the > function call to drm_crtc_send_vblank_event(). > > Reported-by: BassCheck <bass@buaa.edu.cn> I cannot find this report. This is an open source work and public collaboration. The "Reported-by" usually means that the issue was reported to us, in some way, usually in public. Can we see the report? Otherwise adding non-public, non-verifiable reports is useless and clutters our report-credit-system. Best regards, Krzysztof
On 03/07/2023 05:00, Tuo Li wrote: > Hello, > > Thanks for your reply! The report is publicly available at > https://sites.google.com/view/basscheck/home. And this > patch is from the 8th report on this website. Great, thank you! Best regards, Krzysztof
On 30/06/2023 04:19, Tuo Li wrote: > The variable crtc->state->event is often protected by the lock > crtc->dev->event_lock when is accessed. However, it is accessed as a > condition of an if statement in exynos_drm_crtc_atomic_disable() without > holding the lock: > > if (crtc->state->event && !crtc->state->active) > > However, if crtc->state->event is changed to NULL by another thread right > after the conditions of the if statement is checked to be true, a > null-pointer dereference can occur in drm_crtc_send_vblank_event(): > > e->pipe = pipe; > > To fix this possible null-pointer dereference caused by data race, the > spin lock coverage is extended to protect the if statement as well as the > function call to drm_crtc_send_vblank_event(). > > Reported-by: BassCheck <bass@buaa.edu.cn> > Signed-off-by: Tuo Li <islituo@gmail.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
> -----Original Message----- > From: Tuo Li <islituo@gmail.com> > Sent: Friday, June 30, 2023 11:19 AM > To: inki.dae@samsung.com; sw0312.kim@samsung.com; > kyungmin.park@samsung.com; airlied@gmail.com; daniel@ffwll.ch; > krzysztof.kozlowski@linaro.org; alim.akhtar@samsung.com > Cc: dri-devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; > linux-samsung-soc@vger.kernel.org; linux-kernel@vger.kernel.org; > baijiaju1990@outlook.com; Tuo Li <islituo@gmail.com>; BassCheck > <bass@buaa.edu.cn> > Subject: [PATCH] drm/exynos: fix a possible null-pointer dereference due > to data race in exynos_drm_crtc_atomic_disable() > > The variable crtc->state->event is often protected by the lock > crtc->dev->event_lock when is accessed. However, it is accessed as a > condition of an if statement in exynos_drm_crtc_atomic_disable() without > holding the lock: > > if (crtc->state->event && !crtc->state->active) > > However, if crtc->state->event is changed to NULL by another thread right > after the conditions of the if statement is checked to be true, a > null-pointer dereference can occur in drm_crtc_send_vblank_event(): > > e->pipe = pipe; > > To fix this possible null-pointer dereference caused by data race, the > spin lock coverage is extended to protect the if statement as well as the > function call to drm_crtc_send_vblank_event(). > > Reported-by: BassCheck <bass@buaa.edu.cn> > Signed-off-by: Tuo Li <islituo@gmail.com> Applied. Thanks, Inki Dae > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > index 4153f302de7c..d19e796c2061 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -39,13 +39,12 @@ static void exynos_drm_crtc_atomic_disable(struct > drm_crtc *crtc, > if (exynos_crtc->ops->atomic_disable) > exynos_crtc->ops->atomic_disable(exynos_crtc); > > + spin_lock_irq(&crtc->dev->event_lock); > if (crtc->state->event && !crtc->state->active) { > - spin_lock_irq(&crtc->dev->event_lock); > drm_crtc_send_vblank_event(crtc, crtc->state->event); > - spin_unlock_irq(&crtc->dev->event_lock); > - > crtc->state->event = NULL; > } > + spin_unlock_irq(&crtc->dev->event_lock); > } > > static int exynos_crtc_atomic_check(struct drm_crtc *crtc, > -- > 2.34.1
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 4153f302de7c..d19e796c2061 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -39,13 +39,12 @@ static void exynos_drm_crtc_atomic_disable(struct drm_crtc *crtc, if (exynos_crtc->ops->atomic_disable) exynos_crtc->ops->atomic_disable(exynos_crtc); + spin_lock_irq(&crtc->dev->event_lock); if (crtc->state->event && !crtc->state->active) { - spin_lock_irq(&crtc->dev->event_lock); drm_crtc_send_vblank_event(crtc, crtc->state->event); - spin_unlock_irq(&crtc->dev->event_lock); - crtc->state->event = NULL; } + spin_unlock_irq(&crtc->dev->event_lock); } static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
The variable crtc->state->event is often protected by the lock crtc->dev->event_lock when is accessed. However, it is accessed as a condition of an if statement in exynos_drm_crtc_atomic_disable() without holding the lock: if (crtc->state->event && !crtc->state->active) However, if crtc->state->event is changed to NULL by another thread right after the conditions of the if statement is checked to be true, a null-pointer dereference can occur in drm_crtc_send_vblank_event(): e->pipe = pipe; To fix this possible null-pointer dereference caused by data race, the spin lock coverage is extended to protect the if statement as well as the function call to drm_crtc_send_vblank_event(). Reported-by: BassCheck <bass@buaa.edu.cn> Signed-off-by: Tuo Li <islituo@gmail.com> --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)