On Mon, 18 Mar 2019, Shaobo He <shaobo@xxxxxxxxxxx> 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
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
