[Intel-gfx] Potential NULL pointer dereference in intel_crt_get_edid

[Shaobo He]

Thank you very much for your reply. Do you mean if I track the argument to the 
callers, I should be able to figure out that the pin is always valid? I think I 
have two questions for this approach. First, does it mean that the branch 
returning NULL is practically dead code? Second, the driver code, as it appears 
to me, does not have well-defined entry points like the main function. So if I 
manage to track the arguments to a function whose caller is not explicit (e.g., 
it's address is taken in a structure), should I still assume that the pin is 
valid when this function is used?

Thanks,
Shaobo
On 2019/3/19 2:32, Jani Nikula wrote:
> On Mon, 18 Mar 2019, Shaobo He <shaobo at cs.utah.edu> wrote:
>> I see. In light of this commit, is it a better solution than adding NULL-checks
>> is to replace the if branch conditioned by `WARN_ON` with simply `WARN` like the
>> following,
>>
>> struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
>> 					    unsigned int pin)
>> {
>> 	WARN(!intel_gmbus_is_valid_pin(dev_priv, pin), "Invalid pin: %d\n", pin);
>>
>> 	return &dev_priv->gmbus[pin].adapter;
>> }
> 
> So all of this discussion is hypothetical in the sense that it really
> should never happen. You can track down the args passed to
> intel_gmbus_get_adapter(), while the static analyzer is unable to do
> that.
> 
> BR,
> Jani.
> 
> 
>>
>> Shaobo
>> On 3/18/19 5:53 PM, Rodrigo Vivi wrote:
>>> On Mon, Mar 18, 2019 at 05:39:48PM -0600, Shaobo He wrote:
>>>> Hi Rodrigo,
>>>>
>>>> Sorry I'm a bit lost here. May I ask where the `WARN` is?
>>>
>>> along with the return NULL
>>>
>>> struct i2c_adapter *intel_gmbus_get_adapte()
>>>     if (WARN_ON(!intel_gmbus_is_valid_pin(dev_priv, pin)))
>>>                   return NULL;
>>>
>>>>
>>>> Thanks,
>>>> Shaobo
>>>> On 3/18/19 5:26 PM, Rodrigo Vivi wrote:
>>>>> Hi Shaobo,
>>>>>
>>>>> n Mon, Mar 18, 2019 at 05:01:10PM -0600, Shaobo He wrote:
>>>>>> Hello everyone,
>>>>>>
>>>>>> My name is Shaobo He and I am a graduate student at University of Utah. I am
>>>>>> using a static analysis tool to search for null pointer dereferences and
>>>>>> came across a potentially invalid memory access in the file
>>>>>> drivers/gpu/drm/i915/intel_crt.c: in function `intel_crt_detect_ddc`,
>>>>>> function `intel_gmbus_get_adapter` can return a NULL pointer which is
>>>>>
>>>>> if this happens we've done a terrible job on defining the platform...
>>>>>
>>>>>> dereferenced by the call to `drm_get_edid` or `intel_gmbus_is_forced_bit`.
>>>>>
>>>>> but it seems you are right... this will reach i2c_transfer in the end
>>>>> and it will break everything after we gave the Warning...
>>>>>
>>>>>> It seems that the return value of `intel_gmbus_get_adapter` is never
>>>>>> NULL-checked. If so, it would be better to replace the branch to return a
>>>>>> NULL pointer with something like `BUG_ON`.
>>>>>
>>>>> what about just adding if (!i2c) return false
>>>>> instead of BUG.
>>>>>
>>>>> We already have the WARN to debug if this case ever happens.
>>>>>
>>>>> Thanks,
>>>>> Rodrigo.
>>>>>
>>>>>>
>>>>>> Please let me know if it makes sense. I am looking forward to your reply.
>>>>>>
>>>>>> Best,
>>>>>> Shaobo
>