Re: [PATCH] drm/i915/hdmi: Fix potential overflow while using intel_encoder_to_tc

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 9/3/2025 1:07 PM, Jani Nikula wrote:
> On Wed, 03 Sep 2025, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> The helper intel_encoder_to_tc() can potentially return TC_PORT_NONE
>> (-1) and cause overflow while computing ddc pins in
>> icl_encoder_to_ddc_pin().
>>
>> Check for TC_PORT_NONE before deriving the ddc pins for TC port.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_hdmi.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> index cbee628eb26b..85f70cedc40c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> @@ -2791,10 +2791,16 @@ static u8 icl_encoder_to_ddc_pin(struct intel_encoder *encoder)
>>   	struct intel_display *display = to_intel_display(encoder);
>>   	enum port port = encoder->port;
>>   
>> -	if (intel_encoder_is_combo(encoder))
>> +	if (intel_encoder_is_combo(encoder)) {
>>   		return GMBUS_PIN_1_BXT + port;
>> -	else if (intel_encoder_is_tc(encoder))
>> -		return GMBUS_PIN_9_TC1_ICP + intel_encoder_to_tc(encoder);
>> +	} else if (intel_encoder_is_tc(encoder)) {
>> +		enum tc_port tc_port = intel_encoder_to_tc(encoder);
> intel_encoder_to_tc() can only return TC_PORT_NONE if intel_phy_is_tc()
> == false. But intel_encoder_is_tc() just above means intel_phy_is_tc()
> == true.
>
> This case can't happen, it's a static analyzer being overzealous.

Agreed, in this case tc_port is valid, I can see that now.


>
> Adding checks like this to please a static analyzer leads to misery,
> because it adds unnecessary code to maintain, and it will never be run.
>
> I think it would be more interesting to make intel_port_to_tc() WARN on
> !intel_phy_is_tc(), return a bogus >= 0 port, and ensure all callers
> only call it on TC ports.

Hmm.. I can try this out. Can change intel_port_to_tc() as suggested and 
ensure the callers call intel_phy_is_tc(),

before calling intel_port_to_tc()/intel_encoder_to_tc().

There are few places where the callers of intel_port_to_tc() and 
intel_encoder_to_tc() specifically check for TC_PORT_NONE,

so those places need to be changed as well.


Thanks for the suggestions and comments. I will drop this patch, and 
will prepare for the suggested changes.

Regards,

Ankit

>
> This obviously leads to issues if it happens, but hey, it shouldn't
> happen, and intel_encoder_to_tc() returning TC_PORT_NONE is *already*
> such a case. Just move it to lower levels.
>
> If we start checking for every impossible situation, and propagating
> errors for them, our codebase will be 90% error handling.
>
>
> BR,
> Jani.
>
>
>> +
>> +		if (tc_port != TC_PORT_NONE)
>> +			return GMBUS_PIN_9_TC1_ICP + tc_port;
>> +
>> +		drm_WARN(display->drm, 1, "Invalid TC port\n");
>> +	}
>>   
>>   	drm_WARN(display->drm, 1, "Unknown port:%c\n", port_name(port));
>>   	return GMBUS_PIN_2_BXT;