[Intel-gfx] 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 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
> >