On Mon, Oct 7, 2013 at 2:14 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > 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. > Right. Thanks for clarifying. > Also I'm not sure that AND'ing with 0xff is the right thing to do here. Well, maybe not. I'm not entirely convinced it's *not*, though. >> > Furthermore setting up a bitfield makes >> > the code more complex than it needs to be. >> >> Doesn't this perform worse than the current implementation? Going from >> 1 to 13 checks per write sounds less than ideal to me... > > 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. That's an AND and a shift, not division and modulo, both single-cycle instructions on ARM. I'm pretty sure that'd be a win. > 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. Perhaps the bitmap could be generated compile-time and stuck in read-only memory? > I guess one could actually go and run a benchmark against both versions > and balance the performance impact against the possible security > implications. But given that we don't really have any benchmarks that's > pretty hard to do. Well, perhaps we could have both? :) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
