[PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup

[Stefan Mavrodiev]

It's possible hdmi->connector and hdmi->encoder divices to be NULL.
This can happen when building as kernel module and you try to remove
the module.

This patch make simple null check, before calling the cleanup functions.

Signed-off-by: Stefan Mavrodiev <ste...@olimex.com>
---
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index a7c4654445c7..b61e00f2ecb8 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -685,8 +685,10 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);

cec_unregister_adapter(hdmi->cec_adap);
- drm_connector_cleanup(&hdmi->connector);
- drm_encoder_cleanup(&hdmi->encoder);
+ if (hdmi->connector.dev)
+ drm_connector_cleanup(&hdmi->connector);
+ if (hdmi->encoder.dev)
+ drm_encoder_cleanup(&hdmi->encoder);
i2c_del_adapter(hdmi->i2c);
i2c_put_adapter(hdmi->ddc_i2c);
clk_disable_unprepare(hdmi->mod_clk);
--
2.17.1

[Stefan Mavrodiev]

Hi,

On 12/16/19 6:12 PM, Maxime Ripard wrote:
> Hi,

>
> On Mon, Dec 16, 2019 at 04:43:48PM +0200, Stefan Mavrodiev wrote:
>> It's possible hdmi->connector and hdmi->encoder divices to be NULL.
>> This can happen when building as kernel module and you try to remove
>> the module.
>>
>> This patch make simple null check, before calling the cleanup functions.
>>
>> Signed-off-by: Stefan Mavrodiev <ste...@olimex.com>
>> ---
>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index a7c4654445c7..b61e00f2ecb8 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -685,8 +685,10 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
>> struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);
>>
>> cec_unregister_adapter(hdmi->cec_adap);
>> - drm_connector_cleanup(&hdmi->connector);
>> - drm_encoder_cleanup(&hdmi->encoder);
>> + if (hdmi->connector.dev)
>> + drm_connector_cleanup(&hdmi->connector);
>> + if (hdmi->encoder.dev)
>> + drm_encoder_cleanup(&hdmi->encoder);

> Hmmm, this doesn't look right. Do you have more information on how you
> can reproduce it?

Just build sun4i_drm_hdmi as module (CONFIG_DRM_SUN4I_HDMI=m). Then try
to unload the module:

# rmmod sun4i_drm_hdmi

And you get this:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = 6b032436
[00000000] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in: sun4i_drm_hdmi(-)
CPU: 0 PID: 1081 Comm: rmmod Not tainted 5.5.0-rc1-00030-g6ec417030d93 #33
Hardware name: Allwinner sun7i (A20) Family
PC is at drm_connector_cleanup+0x40/0x208
LR is at sun4i_hdmi_unbind+0x10/0x54 [sun4i_drm_hdmi]
...


I've tested that with sunxi/for-next branch on A20-OLinuXino board.

Best regards,
Stefan

>
> Maxime

[Uwe Kleine-König]

On Mon, Dec 16, 2019 at 04:43:48PM +0200, Stefan Mavrodiev wrote:

> It's possible hdmi->connector and hdmi->encoder divices to be NULL.

The wording is broken and s/divices/devices/. I'd write:

It's possible that the parent devices of the connector and/or
encoder are NULL. This makes drm_connector_cleanup and
drm_encoder_cleanup respectively trigger a NULL pointer
exeception:

<...log here...>

This is reproducible by

<...receipt here...>

So add a check for NULL before calling these functions.

Of course this doesn't address the reservations by Maxime.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

[Stefan Mavrodiev]

Hi,

On 12/17/19 1:49 PM, Maxime Ripard wrote:
> On Tue, Dec 17, 2019 at 12:46:03PM +0100, Maxime Ripard wrote:

>> Yeah, you detailed the symptoms nicely in the commit log, but my point
>> was that we shouldn't end up in that situation in the first place.
>>
>> Your patch works around it, but it doesn't fix the underlying
>> issue. Is drm_connector_cleanup (or the encoder variant) called twice?
> Answering myself, yes it is. It's both the destroy call back and
> called in unbind. We should just remove the one in the unbind then.

Should I do this or leave it to you?

Stefan

>
> Maxime