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

[Terje Bergström]

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 at gmail.com> 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.

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.

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

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

Terje