Potential NULL pointer dereference in intel_crt_get_edid

Potential NULL pointer dereference in intel_crt_get_edid

[Shaobo He]

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 dereferenced by the 
call to `drm_get_edid` or `intel_gmbus_is_forced_bit`. 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`.

Please let me know if it makes sense. I am looking forward to your reply.

Best,
Shaobo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Potential NULL pointer dereference in intel_crt_get_edid

[Rodrigo Vivi]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Potential NULL pointer dereference in intel_crt_get_edid

[Shaobo He]

Hi Rodrigo,

Sorry I'm a bit lost here. May I ask where the `WARN` is?

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Potential NULL pointer dereference in intel_crt_get_edid

[Rodrigo Vivi]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Potential NULL pointer dereference in intel_crt_get_edid

[Shaobo He]

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;
}

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Potential NULL pointer dereference in intel_crt_get_edid

[Jani Nikula]

On Mon, 18 Mar 2019, Shaobo He <shaobo@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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: 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@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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Potential NULL pointer dereference in intel_crt_get_edid

[Rodrigo Vivi]

On Tue, Mar 19, 2019 at 10:53:53AM -0600, Shaobo He wrote:
> 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?

Yes, that's the case.

Only possibility that we could reach that is if when we are adding
a new platform but we make some mistake on defining the pin table.
so it is there just in case.

> Second, the driver code, as
> it appears to me, does not have well-defined entry points like the main
> function.

welcome! :)

it is complex indeed. It has the probe and entry points would be the ioctls,
interrups, sysfs, debugfs, etc...

my suggestion is to use something like cscope/ctags to navigate and try to
make some sense of the functions around that particular code you are looking at.

> 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?

on this particular case yes. in general, use the cscope on the vfunc call to see
when it gets called...

> 
> Thanks,
> Shaobo
> On 2019/3/19 2:32, Jani Nikula wrote:
> > On Mon, 18 Mar 2019, Shaobo He <shaobo@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
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx