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@xxxxxxxxx> 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:
pgpfGpQhmzPm5.pgp
Description: PGP signature
