drm/vc4: fix NULL dereference in vc4_hvs_unbind
Subsystems (4)
PATCH 1/1 drm/vc4: fix NULL dereference in vc4_hvs_unbind
With 'dtoverlay=vc4-kms-v3d,noaudio' and 'hdmi=off' on Raspberry Pi, unloading the vc4 module calls vc4_hvs_unbind() with dev_get_drvdata(master) returning NULL.
Return early when 'drm' is NULL before converting it to 'vc4' and before dereferencing 'vc4->hvs', preventing a kernel oops.
--- drivers/gpu/drm/vc4/vc4_hvs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/gpu/drm/vc4/vc4_hvs.c +8 -2
@@ -1753,10 +1753,16 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, void *data) {struct drm_device *drm =dev_get_drvdata (master); -struct vc4_dev *vc4 =to_vc4_dev (drm); -struct vc4_hvs *hvs = vc4->hvs; +struct vc4_dev *vc4; +struct vc4_hvs *hvs;struct drm_mm_node *node, *next; +if (!drm) +return ; + + vc4 =to_vc4_dev (drm); + hvs = vc4->hvs; +if (drm_mm_node_allocated (&vc4->hvs->mitchell_netravali_filter))drm_mm_remove_node (&vc4->hvs->mitchell_netravali_filter);
Dave Stevenson
maintainerHi Thorsten
Quoted 2 lines · Thorsten Blum
With 'dtoverlay=vc4-kms-v3d,noaudio' and 'hdmi=off' on Raspberry Pi,
Mainline doesn't use overlays, so this description isn't valid.
Which generation of Pi are you using? Whilst they all share the vc4 driver, the functionality associated differs. If you're disabling HDMI (and HDMI audio), which display outputs are you using?
Quoted 5 lines
unloading the vc4 module calls
vc4_hvs_unbind()withdev_get_drvdata(master)returning NULL.Return early when 'drm' is NULL before converting it to 'vc4' and before dereferencing '
vc4->hvs', preventing a kernel oops.
That leaves things allocated and clocks running, so bailing out isn't a fix. I'll have a look to see why dev_get_drvdata is returning NULL.
Dave
Quoted 30 lines
Fixes c8b75bca92cb ("drm/vc4: Add KMS support for Raspberry Pi.")Signed-off-by Thorsten Blum <[email protected]>--- drivers/gpu/drm/vc4/vc4_hvs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpu/drm/vc4/vc4_hvs.c +8
-2@@ -1753,10 +1753,16 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, void *data) {struct drm_device *drm =dev_get_drvdata (master); -struct vc4_dev *vc4 =to_vc4_dev (drm); -struct vc4_hvs *hvs = vc4->hvs; +struct vc4_dev *vc4; +struct vc4_hvs *hvs;struct drm_mm_node *node, *next; +if (!drm) +return ; + + vc4 =to_vc4_dev (drm); + hvs = vc4->hvs; +if (drm_mm_node_allocated (&vc4->hvs->mitchell_netravali_filter))drm_mm_remove_node (&vc4->hvs->mitchell_netravali_filter);
Hi Dave,
Quoted 11 lines · Dave Stevenson
Hi Thorsten
Quoted 2 lines · Thorsten Blum
With 'dtoverlay=vc4-kms-v3d,noaudio' and 'hdmi=off' on Raspberry Pi,
Mainline doesn't use overlays, so this description isn't valid.
Which generation of Pi are you using? Whilst they all share the vc4 driver, the functionality associated differs. If you're disabling HDMI (and HDMI audio), which display outputs are you using?
It's a Pi 500 currently running headless, which is why I turned audio and HDMI off. I ended up using:
dtparam=audio=off
#dtoverlay=vc4-kms-v3d
hdmi=off
This prevents the vc4 and snd modules from loading and works for me.
Quoted 8 lines
Quoted 5 lines
unloading the vc4 module calls
vc4_hvs_unbind()withdev_get_drvdata(master)returning NULL.Return early when 'drm' is NULL before converting it to 'vc4' and before dereferencing '
vc4->hvs', preventing a kernel oops.That leaves things allocated and clocks running, so bailing out isn't a fix. I'll have a look to see why
dev_get_drvdatais returning NULL.
Yes, I realized there are probably other things that need to be fixed. However, the defensive NULL check avoided the kernel oops for me.
Thanks,
Thorsten
Dave Stevenson
maintainerHi Thorsten
Quoted 16 lines · Thorsten Blum
Hi Dave,
Quoted 11 lines · Dave Stevenson
Hi Thorsten
Quoted 2 lines · Thorsten Blum
With 'dtoverlay=vc4-kms-v3d,noaudio' and 'hdmi=off' on Raspberry Pi,
Mainline doesn't use overlays, so this description isn't valid.
Which generation of Pi are you using? Whilst they all share the vc4 driver, the functionality associated differs. If you're disabling HDMI (and HDMI audio), which display outputs are you using?
Quoted 6 lines
It's a Pi 500 currently running headless, which is why I turned audio and HDMI off. I ended up using:
dtparam=audio=off
#dtoverlay=vc4-kms-v3d
hdmi=off
You're still working from our vendor kernel[1] rather than mainline. The mainline kernel doesn't support the Pi500 yet, and doesn't use overlays.
The bug is present in both, but descriptions reported to mainline should correspond to mainline. Otherwise report it to our vendor kernel repo.
There's also no such config.txt parameter as "hdmi=off".
[1] https://github.com/raspberrypi/linux
Quoted 5 lines
This prevents the vc4 and snd modules from loading and works for me.
Quoted 3 lines
Quoted 3 lines
unloading the vc4 module calls
vc4_hvs_unbind()withdev_get_drvdata(master)returning NULL.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Return early when 'drm' is NULL before converting it to 'vc4' and before dereferencing '
vc4->hvs', preventing a kernel oops.That leaves things allocated and clocks running, so bailing out isn't a fix. I'll have a look to see why
dev_get_drvdatais returning NULL.Yes, I realized there are probably other things that need to be fixed. However, the defensive NULL check avoided the kernel oops for me.
A quick check says that vc4_drm_unbind gets called first and sets drvdata to NULL.
The devm action then triggers and calls vc4_component_unbind_all, which in turn calls vc4_hvs_unbind. The relevant pointer is passed to the unbind as "void *data", but not used.
In vc4_hvs_unbind, changing
struct drm_device *drm = dev_get_drvdata(master); to
struct drm_device *drm = (struct drm_device *)data; fixes it for me.
It looks like vc4_v3d (used on Pi 0-3) has the same issue. vc4_crtc and vc4_txp both store drvdata against their own device, not against the master. I'll have a bit of a think as to whether that is better than using the "data" pointer.
AIUI unbinding DRM drivers is an unusual operation anyway. If audio is enabled then I can't "rmmod vc4" as the module is in use, but I can if audio isn't enabled. I can't immediately see a way around that one.
Dave