Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

On Mon, Oct 07, 2013 at 03:52:28PM +0300, Terje Bergström wrote:
> On 07.10.2013 15:14, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Mon, Oct 07, 2013 at 01:34:44PM +0200, Erik Faye-Lund wrote:
> >> On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
> >> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> Rework the address table code for the host1x firewall. The previous
> >>> implementation allocated a bitfield but didn't check for a valid pointer
> >>> so it could potentially crash.
> >>
> >> I don't think it could crash. The bitmaps was allocated as a 256-bit
> >> field, and the register offset gets AND'ed with 0xFF before being
> >> looked up. What am I missing?
> > 
> > The pointer returned from the allocation is never checked, so it could
> > theoretically be NULL and be used regardless.
> > 
> > Also I'm not sure that AND'ing with 0xff is the right thing to do here.
> 
> Oops, there's a check for NULL missing. If that allocation fails, probe
> should fail, so we just need to propagate the error condition. Otherwise
> we might just leave the firewall off, and let everything go in unchecked.

Yes, definitely.

> AND 0xff is necessary, because the same registers are mirrored in
> multiple contexts. AND removes the offset coming from context, and
> leaves just the plain register offset.

That looks like something which should go into a comment.

> > I'm not so sure. Caching should alleviate the issue with the increased
> > amount of data. Then there's the fact that previously we needed to
> > divide and compute the remainder of the division for the BIT_MASK and
> > BIT_WORD operations.
> 
> Don't these get compiled into shifts and bitwise ands?

Yes, they are.

> > One other added benefit of this approach is that the address registers
> > are stored in a const array and therefore could reside in a read-only
> > memory region. With the previous approach, once somebody had access to
> > the bitmap, it could easily be overwritten with zeros and effectively
> > make the firewall useless. I'm not sure how likely that would be, but it
> > could be done.
> 
> If somebody gets access to the bitmap, he has access to kernel data
> structures. GR2D firewall is the least of our worries in this case.

I see the point. Oh well, all my arguments are torn down. I'll drop this
patch. Or at least the part that rewrites the gr2d_is_addr() and make it
check for failed allocations properly.

For what it's worth, I still think the plain table lookup is much easier
to understand.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]