On Mon, Oct 7, 2013 at 2:14 PM, Thierry Reding thierry.reding@gmail.com 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@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.
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? :)