[Intel-gfx] [PATCH] drm/i915: Check DVO reads for errors

[Quentin Casasnovas]

On Thu, Feb 26, 2015 at 10:09:49PM +0000, Chris Wilson wrote:
> On Thu, Feb 26, 2015 at 06:47:14PM +0100, Quentin Casasnovas wrote:
> > On Thu, Feb 26, 2015 at 05:10:17PM +0000, Chris Wilson wrote:
> > > Not all of the DVO functions were checking the return value from their
> > > i2c routines when reading registers. This could lead to us feeding
> > > garbage values back into the hardware, possible causing further
> > > failures. In some cases the uninitialised stack values were being
> > > written into the kernel log.
> > > 
> > > Quentin Casasnovas suggested the simple solution of just initialising
> > > the output parameter to zero in all cases, but we may as well spend the
> > > extra few moments to fix it correctly.
> > 
> > I'm not sure your patch would be -stable material mainly because of the
> > diffstat.  Given the security implications, I would still rather have my
> > patch merged first so it can easily be back-ported to -stable and distro
> > kernels easily, and then have your patch on top when it gets properly
> > reviewed.  Especially since your patch looks like it's doing other
> > not strictly related stuffs like these:
> 
> I don't agree that your patch is stable material since it is not
> obviously correct (it potentially changes values written to hw), hasn't
> been tested and doesn't qualify as a "real bug that impacts people".
>

I must admit I'm not quite sure what you mean by saying it potentially
changes values written to the hardware.  IIRC, in the current code, we
might be writting garbage values from the stack to the hardware (so hard to
tell in that case what is it we are writting..), and use that junk to test
some flags/print some debug output.  I thought my patch was helping in that
matter by at least not using/printing that junk.

> To fix the security concerns you expressed, you could have equally
> removed the DRM_DEBUG_KMS().

Agreed, though we'd still be using some garbage values in some conditional
expressions and potentially write them back to the hardware.  But yeah I'm
no expert so up to you really, I just thought your patch was quite big and
still maybe not complete since it does check every *read() return values -
we might as well take my approach as a first baby step and you can then go
ahead and properly fix the callers to check for the return value?

Not saying this is critical material anyway, just that the code looked
wrong so I tried a minimal fix so it could be easily backported.

Quentin