[RFC] docs: Specify V3 of the mbox protocol

[Suraj Jitindar Singh]

@wak ping,

Thoughts on using the first arg in the response of get flash name as a
length argument?

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