| Message ID | 20241031105145.2140590-1-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] drm: i915: do not NULL deref hdmi attached_connector | expand |
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) {
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.
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?
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.
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
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.
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) {
*** 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(-)