[RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Sergey Senozhatsky]

	*** RFC ***

intel_ddi_init() may skip connector initialization, for instance,
both intel_ddi_init_dp_connector() and intel_ddi_init_hdmi_connector()
are optional.  This leads to situation that ->attached_connector may
be NULL for some connectors.  For instance, on my setup 'DDI A/PHY A'
and 'DDI TC1/PHY TC1' are not initialized.

However, functions like intel_dp_dual_mode_set_tmds_output() and
friends don't take this into consideration.  This leads to NULL
ptr-derefs:

KASAN: null-ptr-deref in range [0x0000000000000848-0x000000000000084f]
RIP: 0010:intel_hdmi_encoder_shutdown+0x105/0x230
Call Trace:
<TASK>
i915_driver_shutdown+0x2d8/0x490
pci_device_shutdown+0x83/0x150
device_shutdown+0x4ad/0x660
__se_sys_reboot+0x29c/0x4d0
do_syscall_64+0x60/0x90

Add a new helper to avoid NULL ->attached_connector derefs and
switch some intel_hdmi function to it.  I'm not sure if we need
to switch all or just intel_dp_dual_mode_set_tmds_output() (I
have only seen this one doing NULL derefs so far).

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 27 ++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index e1a1351bc94f..c089dd20972b 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -1256,12 +1256,19 @@ static void hsw_set_infoframes(struct intel_encoder *encoder,
 			      &crtc_state->infoframes.drm);
 }
 
+static struct i2c_adapter *to_ddc(struct intel_hdmi *hdmi)
+{
+	if (hdmi->attached_connector)
+		return hdmi->attached_connector->base.ddc;
+	return NULL;
+}
+
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable)
 {
 	struct intel_display *display = to_intel_display(hdmi);
-	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
+	struct i2c_adapter *ddc = to_ddc(hdmi);
 
-	if (hdmi->dp_dual_mode.type < DRM_DP_DUAL_MODE_TYPE2_DVI)
+	if (!ddc || hdmi->dp_dual_mode.type < DRM_DP_DUAL_MODE_TYPE2_DVI)
 		return;
 
 	drm_dbg_kms(display->drm, "%s DP dual mode adaptor TMDS output\n",
@@ -1275,7 +1282,7 @@ static int intel_hdmi_hdcp_read(struct intel_digital_port *dig_port,
 				unsigned int offset, void *buffer, size_t size)
 {
 	struct intel_hdmi *hdmi = &dig_port->hdmi;
-	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
+	struct i2c_adapter *ddc = to_ddc(hdmi);
 	int ret;
 	u8 start = offset & 0xff;
 	struct i2c_msg msgs[] = {
@@ -1292,6 +1299,10 @@ static int intel_hdmi_hdcp_read(struct intel_digital_port *dig_port,
 			.buf = buffer
 		}
 	};
+
+	if (!ddc)
+		return -EINVAL;
+
 	ret = i2c_transfer(ddc, msgs, ARRAY_SIZE(msgs));
 	if (ret == ARRAY_SIZE(msgs))
 		return 0;
@@ -1302,11 +1313,14 @@ static int intel_hdmi_hdcp_write(struct intel_digital_port *dig_port,
 				 unsigned int offset, void *buffer, size_t size)
 {
 	struct intel_hdmi *hdmi = &dig_port->hdmi;
-	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
+	struct i2c_adapter *ddc = to_ddc(hdmi);
 	int ret;
 	u8 *write_buf;
 	struct i2c_msg msg;
 
+	if (!ddc)
+		return -EINVAL;
+
 	write_buf = kzalloc(size + 1, GFP_KERNEL);
 	if (!write_buf)
 		return -ENOMEM;
@@ -1335,9 +1349,12 @@ int intel_hdmi_hdcp_write_an_aksv(struct intel_digital_port *dig_port,
 {
 	struct intel_display *display = to_intel_display(dig_port);
 	struct intel_hdmi *hdmi = &dig_port->hdmi;
-	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
+	struct i2c_adapter *ddc = to_ddc(hdmi);
 	int ret;
 
+	if (!ddc)
+		return -EINVAL;
+
 	ret = intel_hdmi_hdcp_write(dig_port, DRM_HDCP_DDC_AN, an,
 				    DRM_HDCP_AN_LEN);
 	if (ret) {
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Jani Nikula]

On Thu, 31 Oct 2024, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> 	*** RFC ***
>
> intel_ddi_init() may skip connector initialization, for instance,
> both intel_ddi_init_dp_connector() and intel_ddi_init_hdmi_connector()
> are optional.  This leads to situation that ->attached_connector may
> be NULL for some connectors.  For instance, on my setup 'DDI A/PHY A'
> and 'DDI TC1/PHY TC1' are not initialized.
>
> However, functions like intel_dp_dual_mode_set_tmds_output() and
> friends don't take this into consideration.  This leads to NULL
> ptr-derefs:
>
> KASAN: null-ptr-deref in range [0x0000000000000848-0x000000000000084f]
> RIP: 0010:intel_hdmi_encoder_shutdown+0x105/0x230
> Call Trace:
> <TASK>
> i915_driver_shutdown+0x2d8/0x490
> pci_device_shutdown+0x83/0x150
> device_shutdown+0x4ad/0x660
> __se_sys_reboot+0x29c/0x4d0
> do_syscall_64+0x60/0x90
>
> Add a new helper to avoid NULL ->attached_connector derefs and
> switch some intel_hdmi function to it.  I'm not sure if we need
> to switch all or just intel_dp_dual_mode_set_tmds_output() (I
> have only seen this one doing NULL derefs so far).

I think the question is, what are we doing running this code if the
connector initialization was skipped?

BR,
Jani.

>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 27 ++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index e1a1351bc94f..c089dd20972b 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1256,12 +1256,19 @@ static void hsw_set_infoframes(struct intel_encoder *encoder,
>  			      &crtc_state->infoframes.drm);
>  }
>  
> +static struct i2c_adapter *to_ddc(struct intel_hdmi *hdmi)
> +{
> +	if (hdmi->attached_connector)
> +		return hdmi->attached_connector->base.ddc;
> +	return NULL;
> +}
> +
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable)
>  {
>  	struct intel_display *display = to_intel_display(hdmi);
> -	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
> +	struct i2c_adapter *ddc = to_ddc(hdmi);
>  
> -	if (hdmi->dp_dual_mode.type < DRM_DP_DUAL_MODE_TYPE2_DVI)
> +	if (!ddc || hdmi->dp_dual_mode.type < DRM_DP_DUAL_MODE_TYPE2_DVI)
>  		return;
>  
>  	drm_dbg_kms(display->drm, "%s DP dual mode adaptor TMDS output\n",
> @@ -1275,7 +1282,7 @@ static int intel_hdmi_hdcp_read(struct intel_digital_port *dig_port,
>  				unsigned int offset, void *buffer, size_t size)
>  {
>  	struct intel_hdmi *hdmi = &dig_port->hdmi;
> -	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
> +	struct i2c_adapter *ddc = to_ddc(hdmi);
>  	int ret;
>  	u8 start = offset & 0xff;
>  	struct i2c_msg msgs[] = {
> @@ -1292,6 +1299,10 @@ static int intel_hdmi_hdcp_read(struct intel_digital_port *dig_port,
>  			.buf = buffer
>  		}
>  	};
> +
> +	if (!ddc)
> +		return -EINVAL;
> +
>  	ret = i2c_transfer(ddc, msgs, ARRAY_SIZE(msgs));
>  	if (ret == ARRAY_SIZE(msgs))
>  		return 0;
> @@ -1302,11 +1313,14 @@ static int intel_hdmi_hdcp_write(struct intel_digital_port *dig_port,
>  				 unsigned int offset, void *buffer, size_t size)
>  {
>  	struct intel_hdmi *hdmi = &dig_port->hdmi;
> -	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
> +	struct i2c_adapter *ddc = to_ddc(hdmi);
>  	int ret;
>  	u8 *write_buf;
>  	struct i2c_msg msg;
>  
> +	if (!ddc)
> +		return -EINVAL;
> +
>  	write_buf = kzalloc(size + 1, GFP_KERNEL);
>  	if (!write_buf)
>  		return -ENOMEM;
> @@ -1335,9 +1349,12 @@ int intel_hdmi_hdcp_write_an_aksv(struct intel_digital_port *dig_port,
>  {
>  	struct intel_display *display = to_intel_display(dig_port);
>  	struct intel_hdmi *hdmi = &dig_port->hdmi;
> -	struct i2c_adapter *ddc = hdmi->attached_connector->base.ddc;
> +	struct i2c_adapter *ddc = to_ddc(hdmi);
>  	int ret;
>  
> +	if (!ddc)
> +		return -EINVAL;
> +
>  	ret = intel_hdmi_hdcp_write(dig_port, DRM_HDCP_DDC_AN, an,
>  				    DRM_HDCP_AN_LEN);
>  	if (ret) {

-- 
Jani Nikula, Intel

Re: [RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Sergey Senozhatsky]

On (24/10/31 13:33), Jani Nikula wrote:
> > intel_ddi_init() may skip connector initialization, for instance,
> > both intel_ddi_init_dp_connector() and intel_ddi_init_hdmi_connector()
> > are optional.  This leads to situation that ->attached_connector may
> > be NULL for some connectors.  For instance, on my setup 'DDI A/PHY A'
> > and 'DDI TC1/PHY TC1' are not initialized.
> >
> > However, functions like intel_dp_dual_mode_set_tmds_output() and
> > friends don't take this into consideration.  This leads to NULL
> > ptr-derefs:
> >
> > KASAN: null-ptr-deref in range [0x0000000000000848-0x000000000000084f]
> > RIP: 0010:intel_hdmi_encoder_shutdown+0x105/0x230
> > Call Trace:
> > <TASK>
> > i915_driver_shutdown+0x2d8/0x490
> > pci_device_shutdown+0x83/0x150
> > device_shutdown+0x4ad/0x660
> > __se_sys_reboot+0x29c/0x4d0
> > do_syscall_64+0x60/0x90
> >
> > Add a new helper to avoid NULL ->attached_connector derefs and
> > switch some intel_hdmi function to it.  I'm not sure if we need
> > to switch all or just intel_dp_dual_mode_set_tmds_output() (I
> > have only seen this one doing NULL derefs so far).
> 
> I think the question is, what are we doing running this code if the
> connector initialization was skipped?

I'm not in position to answer that question, I guess it wasn't even
asked to me.  But...

For instance, intel_ddi_init_hdmi_connector()->intel_hdmi_init_connector()
can "error out" and leave ->attached_connector NULL; I can count 3
conditional returns before `->attached_connector = intel_connector`
assignment, yet none of the upper functions would even know, because
intel_hdmi_init_connector() returns void.  And this is not the only case.
So there are several ways to have ->attached_connector == NULL.

Re: [RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Sergey Senozhatsky]

On (24/10/31 19:51), Sergey Senozhatsky wrote:
> intel_ddi_init() may skip connector initialization, for instance,
> both intel_ddi_init_dp_connector() and intel_ddi_init_hdmi_connector()
> are optional.  This leads to situation that ->attached_connector may
> be NULL for some connectors.  For instance, on my setup 'DDI A/PHY A'
> and 'DDI TC1/PHY TC1' are not initialized.
> 
> However, functions like intel_dp_dual_mode_set_tmds_output() and
> friends don't take this into consideration.  This leads to NULL
> ptr-derefs:
> 
> KASAN: null-ptr-deref in range [0x0000000000000848-0x000000000000084f]
> RIP: 0010:intel_hdmi_encoder_shutdown+0x105/0x230
> Call Trace:
> <TASK>
> i915_driver_shutdown+0x2d8/0x490
> pci_device_shutdown+0x83/0x150
> device_shutdown+0x4ad/0x660
> __se_sys_reboot+0x29c/0x4d0
> do_syscall_64+0x60/0x90
> 
> Add a new helper to avoid NULL ->attached_connector derefs and
> switch some intel_hdmi function to it.  I'm not sure if we need
> to switch all or just intel_dp_dual_mode_set_tmds_output() (I
> have only seen this one doing NULL derefs so far).

Folks, any more comments / opinions on this?
What should be the way forward?

Re: [RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Jani Nikula]

On Wed, 13 Nov 2024, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> On (24/10/31 19:51), Sergey Senozhatsky wrote:
>> intel_ddi_init() may skip connector initialization, for instance,
>> both intel_ddi_init_dp_connector() and intel_ddi_init_hdmi_connector()
>> are optional.  This leads to situation that ->attached_connector may
>> be NULL for some connectors.  For instance, on my setup 'DDI A/PHY A'
>> and 'DDI TC1/PHY TC1' are not initialized.
>> 
>> However, functions like intel_dp_dual_mode_set_tmds_output() and
>> friends don't take this into consideration.  This leads to NULL
>> ptr-derefs:
>> 
>> KASAN: null-ptr-deref in range [0x0000000000000848-0x000000000000084f]
>> RIP: 0010:intel_hdmi_encoder_shutdown+0x105/0x230
>> Call Trace:
>> <TASK>
>> i915_driver_shutdown+0x2d8/0x490
>> pci_device_shutdown+0x83/0x150
>> device_shutdown+0x4ad/0x660
>> __se_sys_reboot+0x29c/0x4d0
>> do_syscall_64+0x60/0x90
>> 
>> Add a new helper to avoid NULL ->attached_connector derefs and
>> switch some intel_hdmi function to it.  I'm not sure if we need
>> to switch all or just intel_dp_dual_mode_set_tmds_output() (I
>> have only seen this one doing NULL derefs so far).
>
> Folks, any more comments / opinions on this?
> What should be the way forward?

Ville, we handle intel_ddi_init_dp_connector() failures but not
intel_ddi_init_hdmi_connector() failures. Do you recall if there's a
reason for that? Something like a dual-mode port where DP works but HDMI
gets rejected because of bogus VBT info?

My gut feeling is to propagate errors from intel_hdmi_init_connector()
and handle them properly in g4x_hdmi_init() and
intel_ddi_init_hdmi_connector().

Of course, we have cases where hdmi is just not initialized on DDI, and
those should be handled. But I don't think hdmi->attached_connector !=
NULL is really the right check for that.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Jani Nikula]

On Wed, 13 Nov 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 13 Nov 2024, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
>> On (24/10/31 19:51), Sergey Senozhatsky wrote:
>>> intel_ddi_init() may skip connector initialization, for instance,
>>> both intel_ddi_init_dp_connector() and intel_ddi_init_hdmi_connector()
>>> are optional.  This leads to situation that ->attached_connector may
>>> be NULL for some connectors.  For instance, on my setup 'DDI A/PHY A'
>>> and 'DDI TC1/PHY TC1' are not initialized.
>>> 
>>> However, functions like intel_dp_dual_mode_set_tmds_output() and
>>> friends don't take this into consideration.  This leads to NULL
>>> ptr-derefs:
>>> 
>>> KASAN: null-ptr-deref in range [0x0000000000000848-0x000000000000084f]
>>> RIP: 0010:intel_hdmi_encoder_shutdown+0x105/0x230
>>> Call Trace:
>>> <TASK>
>>> i915_driver_shutdown+0x2d8/0x490
>>> pci_device_shutdown+0x83/0x150
>>> device_shutdown+0x4ad/0x660
>>> __se_sys_reboot+0x29c/0x4d0
>>> do_syscall_64+0x60/0x90
>>> 
>>> Add a new helper to avoid NULL ->attached_connector derefs and
>>> switch some intel_hdmi function to it.  I'm not sure if we need
>>> to switch all or just intel_dp_dual_mode_set_tmds_output() (I
>>> have only seen this one doing NULL derefs so far).
>>
>> Folks, any more comments / opinions on this?
>> What should be the way forward?
>
> Ville, we handle intel_ddi_init_dp_connector() failures but not
> intel_ddi_init_hdmi_connector() failures. Do you recall if there's a
> reason for that? Something like a dual-mode port where DP works but HDMI
> gets rejected because of bogus VBT info?
>
> My gut feeling is to propagate errors from intel_hdmi_init_connector()
> and handle them properly in g4x_hdmi_init() and
> intel_ddi_init_hdmi_connector().
>
> Of course, we have cases where hdmi is just not initialized on DDI, and
> those should be handled. But I don't think hdmi->attached_connector !=
> NULL is really the right check for that.

I'm hoping [1] would solve the issue.

BR,
Jani.

[1] https://lore.kernel.org/r/cover.1731599468.git.jani.nikula@intel.com


-- 
Jani Nikula, Intel

Re: [RFC][PATCH] drm: i915: do not NULL deref hdmi attached_connector

[Sergey Senozhatsky]

On (24/11/14 17:53), Jani Nikula wrote:
> > Ville, we handle intel_ddi_init_dp_connector() failures but not
> > intel_ddi_init_hdmi_connector() failures. Do you recall if there's a
> > reason for that? Something like a dual-mode port where DP works but HDMI
> > gets rejected because of bogus VBT info?
> >
> > My gut feeling is to propagate errors from intel_hdmi_init_connector()
> > and handle them properly in g4x_hdmi_init() and
> > intel_ddi_init_hdmi_connector().
> >
> > Of course, we have cases where hdmi is just not initialized on DDI, and
> > those should be handled. But I don't think hdmi->attached_connector !=
> > NULL is really the right check for that.
> 
> I'm hoping [1] would solve the issue.

Hi Jani, I'll reply in that new thread.