[PATCH] drm/vc4: Fix atomicity violation in vc4_crtc_send_vblank()
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: sta...@vger.kernel.org
Signed-off-by: Qiu-ji Chen
---
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);
+ 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
[PATCH v2] drm/vc4: Fix atomicity violation in vc4_crtc_send_vblank()
An atomicity violation occurs when the vc4_crtc_send_vblank function
executes simultaneously with modifications to crtc->state->event. Consider
a scenario where crtc->state->event is non-null, allowing it to pass the
validity check. However, at the same time, crtc->state->event might be set
to null. In this case, the validity check in vc4_crtc_send_vblank might act
on the old crtc->state->event (before locking), allowing invalid values to
pass the validity check, which could lead to a null pointer dereference.
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.
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: sta...@vger.kernel.org
Signed-off-by: Qiu-ji Chen
---
V2:
The description of the patch has been modified to make it clearer.
Thanks to Simona Vetter for suggesting this improvement.
---
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);
+ 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
Re: [PATCH] drm/vc4: Fix atomicity violation in vc4_crtc_send_vblank()
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
[PATCH] drm/tegra: fix a possible null pointer dereference
In tegra_crtc_reset(), new memory is allocated with kzalloc(), but
no check is performed. Before calling __drm_atomic_helper_crtc_reset,
state should be checked to prevent possible null pointer dereference.
Fixes: b7e0b04ae450 ("drm/tegra: Convert to using
__drm_atomic_helper_crtc_reset() for reset.")
Cc: sta...@vger.kernel.org
Signed-off-by: Qiu-ji Chen
---
drivers/gpu/drm/tegra/dc.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index be61c9d1a4f0..1ed30853bd9e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1388,7 +1388,10 @@ static void tegra_crtc_reset(struct drm_crtc *crtc)
if (crtc->state)
tegra_crtc_atomic_destroy_state(crtc, crtc->state);
- __drm_atomic_helper_crtc_reset(crtc, &state->base);
+ if (state)
+__drm_atomic_helper_crtc_reset(crtc, &state->base);
+ else
+__drm_atomic_helper_crtc_reset(crtc, NULL);
}
static struct drm_crtc_state *
--
2.34.1
[PATCH RESEND] drm/tegra: fix a possible null pointer dereference
In tegra_crtc_reset(), new memory is allocated with kzalloc(), but
no check is performed. Before calling __drm_atomic_helper_crtc_reset,
state should be checked to prevent possible null pointer dereference.
Fixes: b7e0b04ae450 ("drm/tegra: Convert to using
__drm_atomic_helper_crtc_reset() for reset.")
Cc: sta...@vger.kernel.org
Signed-off-by: Qiu-ji Chen
---
drivers/gpu/drm/tegra/dc.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index be61c9d1a4f0..1ed30853bd9e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1388,7 +1388,10 @@ static void tegra_crtc_reset(struct drm_crtc *crtc)
if (crtc->state)
tegra_crtc_atomic_destroy_state(crtc, crtc->state);
- __drm_atomic_helper_crtc_reset(crtc, &state->base);
+ if (state)
+__drm_atomic_helper_crtc_reset(crtc, &state->base);
+ else
+__drm_atomic_helper_crtc_reset(crtc, NULL);
}
static struct drm_crtc_state *
--
2.34.1
