[Skiboot] [RFC] docs: Specify V3 of the mbox protocol

[Suraj Jitindar Singh]

@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