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