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. > > 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. 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. 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. Thierry
Attachment:
pgpiR0sIPmsQC.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
