On Thu, 31 Oct 2024, Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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
