| Message ID | 20240913091053.14220-1-chenqiuji666@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix atomicity violation in vc4_crtc_send_vblank() | expand |
On Fri, Sep 13, 2024 at 05:10:53PM +0800, Qiu-ji Chen wrote: > Atomicity violation occurs when the vc4_crtc_send_vblank function is > executed simultaneously with modifications to crtc->state or > crtc->state->event. Consider a scenario where both crtc->state and > crtc->state->event are non-null. They can pass the validity check, but at > the same time, crtc->state or crtc->state->event could be set to null. In > this case, the validity check in vc4_crtc_send_vblank might act on the old > crtc->state and crtc->state->event (before locking), allowing invalid > values to pass the validity check, leading to null pointer dereference. > > To address this issue, it is recommended to include the validity check of > crtc->state and crtc->state->event within the locking section of the > function. This modification ensures that the values of crtc->state->event > and crtc->state do not change during the validation process, maintaining > their valid conditions. > > This possible bug is found by an experimental static analysis tool > developed by our team. This tool analyzes the locking APIs > to extract function pairs that can be concurrently executed, and then > analyzes the instructions in the paired functions to identify possible > concurrency bugs including data races and atomicity violations. > > Fixes: 68e4a69aec4d ("drm/vc4: crtc: Create vblank reporting function") > Cc: stable@vger.kernel.org > Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com> > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index 8b5a7e5eb146..98885f519827 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -575,10 +575,12 @@ void vc4_crtc_send_vblank(struct drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > unsigned long flags; > > - if (!crtc->state || !crtc->state->event) > + spin_lock_irqsave(&dev->event_lock, flags); crtc->state isn't protected by this spinlock, which also points at the more fundamental bug here: We need to pass the crtc_state from the caller, because those have it (or well, can look it up with drm_atomic_get_new_crtc_state). Then we also do not need a spinlock to protect access to state->event, because in both callers we are the owners of this struct field. -Sima > + if (!crtc->state || !crtc->state->event) { > + spin_unlock_irqrestore(&dev->event_lock, flags); > return; > + } > > - spin_lock_irqsave(&dev->event_lock, flags); > drm_crtc_send_vblank_event(crtc, crtc->state->event); > crtc->state->event = NULL; > spin_unlock_irqrestore(&dev->event_lock, flags); > -- > 2.34.1 >
Hi, In the drm_device structure, it is mentioned: "@event_lock: Protects @vblank_event_list and event delivery in general." I believe that the validity check and the subsequent null assignment operation are part of the event delivery process, and all of these should be protected by the event_lock. If there is no lock protection before the validity check, it is possible for a null crtc->state->event to be passed into the drm_crtc_send_vblank_event() function, leading to a null pointer dereference error. We have observed its callers and found that they are from the drm_crtc_helper_funcs driver interface. We believe that functions within driver interfaces can be concurrent, potentially causing a data race on crtc->state->event. Qiu-ji Chen
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 8b5a7e5eb146..98885f519827 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -575,10 +575,12 @@ void vc4_crtc_send_vblank(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; unsigned long flags; - if (!crtc->state || !crtc->state->event) + spin_lock_irqsave(&dev->event_lock, flags); + if (!crtc->state || !crtc->state->event) { + spin_unlock_irqrestore(&dev->event_lock, flags); return; + } - spin_lock_irqsave(&dev->event_lock, flags); drm_crtc_send_vblank_event(crtc, crtc->state->event); crtc->state->event = NULL; spin_unlock_irqrestore(&dev->event_lock, flags);
Atomicity violation occurs when the vc4_crtc_send_vblank function is executed simultaneously with modifications to crtc->state or crtc->state->event. Consider a scenario where both crtc->state and crtc->state->event are non-null. They can pass the validity check, but at the same time, crtc->state or crtc->state->event could be set to null. In this case, the validity check in vc4_crtc_send_vblank might act on the old crtc->state and crtc->state->event (before locking), allowing invalid values to pass the validity check, leading to null pointer dereference. To address this issue, it is recommended to include the validity check of crtc->state and crtc->state->event within the locking section of the function. This modification ensures that the values of crtc->state->event and crtc->state do not change during the validation process, maintaining their valid conditions. This possible bug is found by an experimental static analysis tool developed by our team. This tool analyzes the locking APIs to extract function pairs that can be concurrently executed, and then analyzes the instructions in the paired functions to identify possible concurrency bugs including data races and atomicity violations. Fixes: 68e4a69aec4d ("drm/vc4: crtc: Create vblank reporting function") Cc: stable@vger.kernel.org Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com> --- drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)