<div dir="ltr">Yeah, go ahead and change it to that</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 2, 2017 at 9:26 PM Suraj Jitindar Singh <<a href="mailto:sjitindarsingh@gmail.com">sjitindarsingh@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">@wak ping,<br>
<br>
Thoughts on using the first arg in the response of get flash name as a<br>
length argument?<br>
<br>
On Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:<br>
> @wak:<br>
><br>
> As per Andrews suggestion below, how do you feel about making the<br>
> first<br>
> arg in the response for get flash name the length?<br>
><br>
> @andrew:<br>
><br>
> Since your comments regarding flash integrity aren't really relevant<br>
> at<br>
> the protocol level do you think it's something which needs to be<br>
> mentioned in this document or is just common sense (unless you're me<br>
> and didn't implement it that way :p)...<br>
><br>
> On Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:<br>
> > > > > +The host may lock an area of flash using the MARK_LOCKED<br>
> > > > > command.<br>
> > > > > Any attempt<br>
> > > > > +to mark dirty or erased this area of flash must fail with<br>
> > > > > the<br>
> > > > > LOCKED_ERROR<br>
> > > > > +response code. The host may open a write window which<br>
> > > > > contains<br>
> > > > > a<br>
> > > > > locked area<br>
> > > > > +of flash however changes to a locked area of flash must<br>
> > > > > never<br>
> > > > > be<br>
> > > > > written back<br>
> > > > > +to the backing data source (i.e. that area of flash must be<br>
> > > > > treated as read<br>
> > > > > +only). An attempt to lock an area of flash which is not<br>
> > > > > clean<br>
> > > > > in<br>
> > > > > the current<br>
> > > > > +window must fail with PARAM_ERROR. (V3)<br>
> > > ><br>
> > > > I think we need to consider how locked regions interact with<br>
> > > > the<br>
> > > > in-<br>
> > > > memory<br>
> > > > caching of the flash data.<br>
> > > ><br>
> > > > The BMC doesn't know that the memory backing the LPC window has<br>
> > > > been<br>
> > > > written until it receives a MARK_DIRTY from the host.<br>
> > > > MARK_DIRTY<br>
> > > > is<br>
> > > > not<br>
> > > > valid on a read window or a locked region, but that doesn't<br>
> > > > mean<br>
> > > > that<br>
> > > > changes to a locked region can't persist in memory due to<br>
> > > > caching<br>
> > > > to<br>
> > > > avoid flash access (which was a part of the motivation for<br>
> > > > memory-<br>
> > > > based <br>
> > > > booting to begin with). This may naively fool host firmware<br>
> > > > into<br>
> > > > thinking it is<br>
> > > > accessing unmolested data as the region is locked, whereas the<br>
> > > > region<br>
> > > > could<br>
> > > > contain anything, just it will never be written to flash.<br>
> > ><br>
> > > Correct<br>
> > ><br>
> > > ><br>
> > > > Should we require window requests intersecting a locked region<br>
> > > > always<br>
> > > > have at<br>
> > > > least the locked region positively match the on-flash data?<br>
> > > > That<br>
> > > > way<br>
> > > > the<br>
> > > > firmware that requested a region be locked could ensure it has<br>
> > > > whatever was on<br>
> > > > the flash at the time it was locked by explicitly closing the<br>
> > > > window<br>
> > > > (read or<br>
> > > > write) once finished with it, which requires a window be opened<br>
> > > > again<br>
> > > > for any<br>
> > > > subsequent access. At that point (open) we can do the locked<br>
> > > > region<br>
> > > > intesection<br>
> > > > calculation and check the data in the window as necessary.<br>
> > ><br>
> > > I guess this brings about an interesting question, when a window<br>
> > > is<br>
> > > requested is it guaranteed that the window provided in return by<br>
> > > the<br>
> > > BMC contains the same contents as the flash for the requested<br>
> > > area?<br>
> ><br>
> > I think expecting otherwise would be strange. What way would the<br>
> > host<br>
> > have to validate that? It could keep its own hashes of window<br>
> > content,<br>
> > but it would need to hash the content itself after opening the<br>
> > window<br>
> > which may be a circular problem (e.g. host reboot if the daemon<br>
> > doesn't<br>
> > invalidate the cache across the reboot operation).<br>
> ><br>
> > > It<br>
> > > would seem intuitively that it should. The first time a window is<br>
> > > opened that is true, but with in memory caching the host could<br>
> > > make<br>
> > > a<br>
> > > change that it never tells the bmc about and the contents<br>
> > > diverge,<br>
> > > and<br>
> > > (assuming the window isn't evicted) persist across window<br>
> > > close/open<br>
> > > calls.<br>
> ><br>
> > Yep.<br>
> ><br>
> > ><br>
> > > Is this the desired behaviour? <br>
> ><br>
> > No, not in my opinion. It doesn't fit the principle of least<br>
> > surprise.<br>
> ><br>
> > > It's kind of the only behaviour which<br>
> > > makes the in-memory caching an effective performance improvement,<br>
> > > if we<br>
> > > reloaded every window every time then we may as well only have<br>
> > > one<br>
> > > window in memory anyway. Maybe we can do this a better way (see<br>
> > > below).<br>
> > ><br>
> > > It's worth noting that the current flash locking daemon<br>
> > > implementation<br>
> > > will explicitly reload an entire window whenever one which<br>
> > > contains<br>
> > > any<br>
> > > locked regions is requested - ensuring (for at least as long as<br>
> > > the<br>
> > > access to the flash device is exclusive) the integrity of the<br>
> > > locked<br>
> > > regions. This could of course be optimised to only reload the<br>
> > > locked<br>
> > > region.<br>
> ><br>
> > Right, and if we take up the techniques discussed below we may even<br>
> > be<br>
> > able to eliminate that.<br>
> ><br>
> > ><br>
> > > ><br>
> > > > Locked regions could thus be a performance hit if we always<br>
> > > > load<br>
> > > > the<br>
> > > > locked<br>
> > > > regions from flash, but surely we prefer (some) integrity over<br>
> > > > performance.<br>
> > > ><br>
> > > > Alternatively, you could cryptographically hash the per-block<br>
> > > > content<br>
> > > > of the<br>
> > > > on-flash locked region during the MARK_LOCKED operation, then<br>
> > > > stash<br>
> > > > the hash<br>
> > > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you<br>
> > > > could<br>
> > > > hash<br>
> > > > any<br>
> > > > intersecting, in-cache locked blocks and compare to the stashed<br>
> > > > hash<br>
> > > > values to<br>
> > > > incur only one set of flash accesses (initial hash<br>
> > > > calculations)<br>
> > > > rather than n.<br>
> > ><br>
> > > This seems like a nice balance between performance and data<br>
> > > integrity.<br>
> > ><br>
> > > I guess this brings about the question again as to whether we<br>
> > > care<br>
> > > about data changes we aren't told about. Do we store a hash of<br>
> > > every<br>
> > > window and before providing a cached copy verify the hash and<br>
> > > reload<br>
> > > the entire window from flash again if there is a discrepancy. The<br>
> > > host<br>
> > > isn't allowed to rely on the persistence of data it never tells<br>
> > > the<br>
> > > BMC<br>
> > > about, but is this something the BMC should be checking?<br>
> ><br>
> > I think so, again in support of the principle of least surprise,<br>
> > and<br>
> > the possible security implications. I proposed verifying the<br>
> > integrity<br>
> > of only the locked regions but it might be worth extending that to<br>
> > entire windows depending on the performance. We should do some<br>
> > measurements.<br>
> ><br>
> > I think implementations also need to be noisy in the case of a<br>
> > discrepancy, logging on the BMC side and possibly issuing a BMC<br>
> > Event<br>
> > (taking another one of our reserved bits for the purpose). The<br>
> > modifications are pretty much limited to accidental scribbling<br>
> > (bugs),<br>
> > or malicious intent (probably enabled by more bugs). Either way,<br>
> > someone should be notified.<br>
> ><br>
> > > > > +Command:<br>
> > > > > > > > + GET_FLASH_NAME (V3)<br>
> > > > > > > > + Arguments:<br>
> > > > > > > > + Args 0: Flash ID<br>
> > > > > > > > + Response:<br>
> > > > ><br>
> > > > > + Args 0-10: Flash Name / UID<br>
> > > ><br>
> > > > Hopefully 11 bytes is enough. Well, 10 I guess - should we<br>
> > > > specify<br>
> > > > that the<br>
> > > > value at index 10 be '\0'? I feel like not requiring that could<br>
> > > > lead<br>
> > > > to<br>
> > > > undesirable implementations.<br>
> > ><br>
> > > Depends on whether we require names be 11 bytes, be null<br>
> > > terminated, or<br>
> > > just contain trailing nulls for names shorter than 11 bytes.<br>
> > ><br>
> > > If we're going to waste an argument on the null terminator we may<br>
> > > as<br>
> > > well just make the first argument the length of the name.<br>
> ><br>
> > I think that's a better idea generally. It doesn't assume C-string<br>
> > semantics, though it's likely that the implementations will require<br>
> > them, which was what I was looking out for.<br>
> ><br>
> > ><br>
> > > Is this something the protocol should care about. Maybe it's just<br>
> > > up to<br>
> > > the daemon and host implementation to put something here that<br>
> > > they<br>
> > > both<br>
> > > understand.<br>
> ><br>
> > Probably not, it was just a thought. It's a balance against<br>
> > avoiding<br>
> > potential vulnerabilities and over-specifying behaviour.<br>
> ><br>
> > ><br>
> > > ><br>
> > > > I guess we don't want to go down the path of continued<br>
> > > > responses<br>
> > > > or<br>
> > > > magic<br>
> > > > windows to accommodate larger names?<br>
> > ><br>
> > > I'd prefer not :)<br>
> > ><br>
> ><br>
> > Same :) Just throwing the question out there for discussion<br>
> ><br>
> > > ><br>
> > > > > > > > + Notes:<br>
> > > > > > > > + Describes a flash with some kind of<br>
> > > > > > > > identifier<br>
> > > > ><br>
> > > > > useful to the<br>
> > > > > > > > + host system. This is typically a null-<br>
> > > > > > > > padded<br>
> > > > ><br>
> > > > > string.<br>
> > > > > +<br>
> > > > > +Command:<br>
> > > > > > > > + MARK_LOCKED (V3)<br>
> > > > > > > > + Arguments:<br>
> > > > > > > > + Args 0-1: Flash offset to lock<br>
> > > > > > > > (blocks)<br>
> > > > ><br>
> > > > > + Args 2-3: Number to lock at offset (blocks)<br>
> > > ><br>
> > > > I guess it's hard to get away from using block-sized<br>
> > > > granuality.<br>
> > > > Do<br>
> > > > we<br>
> > > > currently have any partitions that we might lock that are less<br>
> > > > than a<br>
> > > > block<br>
> > > > size? Or are all partitions expected to be block-size aligned?<br>
> > ><br>
> > > This was the motivation for allowing the host to request a block<br>
> > > size,<br>
> > > so that they can hopefully request something which allows locking<br>
> > > at<br>
> > > the required granularity. Currently all partitions are aligned to<br>
> > > flash<br>
> > > erase size (which is what we set block size to anyway).<br>
> ><br>
> > Okay. It might be worth making a note in the documentation for<br>
> > MBOX_GET_INFO saying as much.<br>
> ><br>
> > ><br>
> > > It's not really worth allowing locking at a finer granularity<br>
> > > than<br>
> > > block size since this is how changes (mark dirty) are specified<br>
> > > and<br>
> > > so<br>
> > > it would be impossible to write back any part of a block if a<br>
> > > single<br>
> > > byte in that block were locked. I'm in favour of using block size<br>
> > > for<br>
> > > now and changing it in a later version if this becomes an issue.<br>
> ><br>
> > Right, it's either blocks for everything or bytes for everything.<br>
> > Consistency is good. I don't think we want to go through the effort<br>
> > of<br>
> > changing that aspect of the spec right now.<br>
> ><br>
> > Cheers,<br>
> ><br>
> > Andrew<br>
</blockquote></div>