RE: [virtio-dev] RE: [PATCH v3] It is a virtio based RPMB (Replay Protected Memory Block) device.

["Huang, Yang" <yang.huang@intel.com>]

> > > > + \newline\url{https://www.nvmexpress.org/wp-content/uploads/NVM_E
> > > > + xpre
> > > > + ss_Revision_1.3.pdf}\\
> > > >
> > > >  \end{longtable}
> > > >
> > >
> > > At this stage I suggest we avoid nvme. Mix of LE and BE in the frame
> > > is just too nasty. And ATM Linux only supports UFS/MMC.
> > >
> >
> > Remove all contents of NVMe RPMB at this phase to simplify the spec?
> 
> Why not.

It's removed in V4.


> > > > +\item[\field{id}] is the identifier of RPMB partitions.
> > >
> > > What does this mean? what are the partitions? Did you mean of the device?
> > >
> > > I suspect what is going on is there's an actual device behind this, and this is
> its ID.
> > > Which again begs the question whether this should just be a part of
> > > a blk or scsi device.
> >
> > It's for multiple RPMB regions. RPMB supports up to 4 regions for UFS 3.0 or
> NVMe 1.3.
> 
> So a device with 1 region it's always 0?
> 

For the device with only 1 region, no such item required.
This is removed in V4 for simplicity. 


> > > > +\devicenormative{\subsection}{Device Initialization}{Device Types
> > > > +/ RPMB Device / Device Initialization}
> > > > +
> > > > +\begin{enumerate}
> > > > +\item The virtqueue is initialized.
> > > > +\item The authentication key of device MUST NOT be programmed at
> > > > +the
> > > first device initialization.
> > >
> > > What is "first device initialization"? The concept does not seem to
> > > apply to virtual devices. How about we just have device
> > > pre-initialized to some key? Or maybe device can be in two states, key
> programming and operation?
> >
> > I will change the statement.
> > Device MUST NOT pre-initialized a RPMB key.
> > The only source of the key is from VIRTIO_RPMB_REQ_PROGRAM_KEY
> request.
> > Before RPMB is programmed, read/write/read write counter will fail with
> 0x0007.
> >
> > >
> > > All conformance clauses belong in conformance sections.  here and
> elsewhere.
> >
> > Thanks. I will add it after the skeleton is stable.
> 
> I'd rather we try to finalize it all in one go, don't have the time to review
> everything twice :)


Sure. It's added to V4 which had been sent out.



> > > > +\item[\field{nonce}] is a random number
> > >
> > >
> > > random how? I think it can just be anything.
> >
> > I will detail it.
> > It used by user who request for read/read_write_counter to protect replay
> attack.
> 
> Oh so if one cares about replay, it needs to be hard to predict.
> 

Yes. This is the responsibility for the caller to fill a random to protect from replay attack.


> > > > +\item[\field{block_count}] is the number of blocks (256B for JEDEC and
> 512B
> > > > +   for NVMe) requested to be read/written.
> > >
> > > can't this be intuited from the buffer size somehow?
> > >
> > > I don't really understand how can you both have block_count and a
> > > fixed size data. how does this make sense?
> >
> > The fixed data refer to the size of one block can be read/written.
> > The block count refers to how many 256 units are requested in one read/write
> request.
> >
> > Example, read request on JEDEC RPMB with two blocks (256B*2):
> > User will send 3 frames, the first is the frame filled by user, with request type,
> address, nonce etc.
> > The last 2 frames are filled by device with data, MAC, nonce, result etc.
> 
> 
> So please map this to virtio buffers and document the mapping.
> 

Not included in V4. Will mention it in V5



> > > > +\item[VIRTIO_RPMB_REQ_RESULT_READ] It is used following with the
> > > > +other
> > > request
> > > > +   types for returned result in one or multiple RPMB frames.
> > >
> > > So each write request must be followed by a status request.  This
> > > just seems like a waste. Why not return the result immediately with
> > > the operation?  Maybe UFS doesn't support this but virtio sure does.
> > >
> > > Does device have to maintain the last request in non-volatile memory
> > > and persist across resets?  You do not list it as being persistent
> > > below. So I would just drop this request type and have driver maintain it in
> guest memory.
> >
> > If VIRTIO_RPMB_REQ_RESULT_READ is not request for write/program_key,
> operation will be performed but no result returned.
> > Itâs followed by write/program_key request in most cases. Device does not
> need to maintain it if user does not request for it.
> > Keeping it for compatibility with hardware does make sense for a unified
> solution regardless bare metal or virtualization.
> 
> 
> 
> So what is wrong with driver doing it?


To answer your previous question firstly:
I don't think the device should maintain the last request in memory and persist across resets. 
Because the VIRTIO_RPMB_REQ_RESULT_READ request is sent to the device together with the other requests like VIRTIO_RPMB_REQ_PROGRAM_KEY or VIRTIO_RPMB_REQ_DATA_WRITE.
The device can check the VIRTIO_RPMB_REQ_RESULT_READ is requested when it receives the two requests.
No such usage that sending VIRTIO_RPMB_REQ_RESULT_READ at first and VIRTIO_RPMB_REQ_RESULT_READ the second for some time.
Because nobody promises no any new requests operated during the time interval.

For your second question:
The requirement always depends on the caller. This aligns with the native, and make the driver pure. 
The driver just does one thing: format the request frames to virtqueue but not access the content of the frame.



> > > > For
> > > > +   VIRTIO_RPMB_REQ_PROGRAM_KEY and
> VIRTIO_RPMB_REQ_DATA_WRITE,
> > > if block count has
> > > > +   not been set to 1 of VIRTIO_RPMB_REQ_RESULT_READ request, then
> > > > +   VIRTIO_RPMB_RES_GENERAL_FAILURE SHALL be indicated.
> > > > +
> > > > +\end{description}
> > > > +If authetication key was programmed successfully, the device
> > > > +SHALL return
> > > with a MAC for any operation requests.
> > >
> > > Lots of typos above make it pretty hard to figure out.
> > >
> > > > +
> > > > +\drivernormative{\subsubsection}{Device Operation}{Device Types /
> > > > +RPMB Device / Device Operation}
> > > > +
> > > > +The driver MUST configure and initialize virtqueue for the requests
> received.
> > >
> > > received where? why isn't it legal to e.g. check feature bits and then reset?
> >
> > Received from other modules who calls virtio rpmb driver.
> > Because RPMB frames are not packed by driver.
> > What driver should do is format the RPMB frames it received and for it to
> virtqueue, then send to device.
> 
> OK pls write this up in the spec.
> 

OK. Not included in V4. Will add it to V5.


> > > > +
> > > > +\devicenormative{\subsubsection}{Device Operation}{Device Types /
> > > > +RPMB Device / Device Operation}
> > > > +
> > > > +The virtio-rpmb device could be backed in a number of ways. It SHOULD
> > > > +   keep consistent behaviors with hardware as described in paragraph
> > > > +   6.6.22 of \hyperref[intro:eMMC]{eMMC}, 12.4 of
> \hyperref[intro:UFS]{UFS}
> > > > +   or 8.10 of \hyperref[intro:NVMe]{NVMe}.
> > > > Some elements are maintained
> > > > +   by the device:
> > > > +\begin{enumerate}
> > > > +\item The device maintains an one time programmable authentication key.
> > > > +   It cannot be overwritten, erased or read. The key is used to
> > > > +   authenticate the accesses when MAC is calculated. This key MUST be
> > > > +   kept regardless of device reset or reboot.
> > > > +\item The device maintains a read-only monotonic write counter. It MUST
> > > > +   be initialized to zero and added by one automatically along with
> > > > +   successful write operation. The value cannot be reset. After
> > > > +   the counter has reached its maximum value 0xFFFF FFFF, it will
> > > > +   not be incremented anymore. This counter MUST be kept regardless
> > > > +   of device reset or reboot.
> > > > +\item The device maintains the data for read/write via authenticated
> > > > +   access.
> > > > +\end{enumerate}
> > > > +
> > >
> > >
> > >
> > > What I have gathered from above is really just a counter and a key
> > > added to a storage device.
> > >
> > > 	Key: you must have the key to write. It's programmed into the
> > > 	device at some point by manufacturer.
> > > 	So later, only someone who got key from manufacturer can write.
> > > 	Data is also authenticated with the key, so if you got
> > > 	the key from manufacturer you can verify it's valid.
> > >
> > > 	What if you didn't get the key but you steal the data
> > > 	in transit to storage device somehow? You could
> > > 	replay it to the device and overwrite valid data.
> > > 	To this end device maintains write counter,
> > > 	so you can't.
> > >
> > > 	What if you can pretend write failed, or filter out writes to pretend
> > > 	they succeeded? First write counter will be wrong, but it's tricky for
> > > 	applications to verify the counter: after all an application can crash.
> > > 	So each write is followed by a response and users must validate MAC to
> > > 	make sure write made it to the device.
> > >
> > >
> > > 	On the other hand, what if again you didn't get the key but you steal
> > > 	the read response?  You could replay that to the application.  To this
> > > 	end read requests include a random value and it's copied to the
> > > 	response. Application can put e.g. a counter there.
> > >
> > >
> > >
> > > I think we are coming back full circle and I would like to ask: so
> > > why does it not make sense to add this functionality to a block
> > > device, or a scsi device? We'd get lots of goodies such as multiqueue, for
> free.
> > >
> > >
> > > If we implement above in a hypervisor in software, we can just
> > > ignore all random hardware limitations, and support e.g. arbitrary
> > > size of writes, arbitrary hash etc etc.
> > >
> > > So I think this is the reason you are making it a separate device
> > > and are trying to support so many messy formats is because you are thinking
> about pass-through.
> > >
> > > However, I note that neither driver nor the hypervisor really care
> > > about things like order of fields or endian-ness.
> > > It's only the MAC calculations that are affected.
> > >
> > > So my suggestion is, for now just define a clean minimal interface,
> > > with some kind of enumerator for an application to discover the
> > > specific MAC used.  And when hypervisor is forwarding the data to
> > > the device, it can move fields around, swap endian-ness or whatever.
> > > Will cost some performance but it does not look like this device is fast path
> anyway.
> > >
> > > And I am guessing most people do trust the hypervisor and so they
> > > will happily just use a clean software implementation.
> > >
> >
> > We can simplify the spec by supporting JEDEC only, single RPMB partition.
> > But compatibility with hardware RPMB will benefit developer/user to
> > maintain a same suite of RPMB APIs,  structures, operations in both
> virtualization and bare metal.
> > We can add some feature bits for extended optimization requirements.
> 
> I don't know, this picks and chooses hardware parts to copy and I'm not sure
> what do you base what to emulate on.
> E.g. doesn't userspace also assume RPMB is part of a storage device?
> 
> It would make sense to me if you just emulated encrypted parts.
> 

What do you refer to, for the encrypted parts?

HV can be regarded as a part of TCB normally.
So HV emulated rpmb device provides guests with security in some content.
RPMB support depends on the some specified media. If HV could emulates it, it provides great flexibility for the Guest.