Re: [PATCH v6 2/9] block: Add encryption context to struct bio

[Christoph Hellwig <hch@infradead.org>]

Hi Eric,

On Wed, Jan 08, 2020 at 09:26:29AM -0800, Eric Biggers wrote:
> The NVMe "key per I/O" draft is heavily flawed, and I don't think it will be
> useful at all in the Linux kernel context.  The problem is that, as far as I
> can tell, it doesn't allow the encryption algorithm and IVs to be selected,
> or even standardized or made discoverable in any way.  It does say that
> AES-256 must be supported, but it doesn't say which mode of operation (i.e.
> it could be something inappropriate for disk encryption, like ECB), nor
> does it say whether AES-256 has to be the default or not, and if it's not
> the default how to discover that and select AES-256.

I've talked to people involved with the TCG side of this spec, where
all the interesting crypto happens.  Currently the plan is to support
KMIP wrapper keys, which specify the exact algorithm and operation
mode, and algorithms and modes for the initial version are planned to
be AES 256/512 XTS.  I also had a chat with an involved person and
they understand the principle that for the inline crypto to be trusted
it needs to be interoperable with (trusted) software algorithms.  So
I don't think it is all doom.

> IV generation is also unspecified, so it
> could be something insecure like always using the same IV.

From talking to one of the initiators of the spec, no it is not intended
to be unspecified, but indeed tied to the LBA (see below).

> Also, since "key per I/O" won't allow selecting IVs, all the encrypted data will
> be tied to its physical location on-disk.  That will make "key per I/O" unusable
> in any case where encrypted blocks are moved without the key, e.g.
> filesystem-level encryption on many filesystems.

File systems don't move data around all that often (saying that with my
fs developer hat on).  In traditional file systems only defragmentation
will move data around, with extent refcounting it can also happen for
dedup, and for file systems that write out of place data gets moved
when parts of a block are rewritten, but in that case a read modify
write cycle is perfomed in the Linux code anyway, so it will go through
the inline encryption engined on the way and the way out.

So in other words - specifying an IV would be useful for some use cases,
but I don't think it is a deal blocker. Even without that is is useful
for block device level encryption, and could have some usefulness for
file system encryption usage.

I think that adding an IV would eventually be useful, but fitting that
into NVMe won't be easy, as you'd need to find a way to specify the IV
for each submission queue entry, which requires growing it, or finding
some way to extend it out of band.

> I've already raised these concerns in the NVMe and TCG Storage working groups,
> and the people working on it refused to make any changes, as they consider "key
> per I/O" to be more akin to the TCG Opal self-encrypting drive specification,
> and not actually intended to be "inline encryption".

While I have my fair share of issues how the spec is developed that
isn't my impression, and at least for the verifyable part I heard
contrary statements.  Feel free to contact me offline to make sure we
can move this into the right direction.

> So let's not over-engineer this kernel patchset to support some broken
> vaporware, please.

Not sharing bio fields for integrity and encryption actually keeps
the patchset simpler (although uses more memory if both options are
enabled).  So my main point here is to not over engineer it for broken
premise that won't be true soon.