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

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Jan 08, 2020 at 06:07:30AM -0800, Christoph Hellwig wrote:
> On Wed, Dec 18, 2019 at 07:47:56PM -0500, Martin K. Petersen wrote:
> > Absolutely. That's why it's a union. Putting your stuff there is a
> > prerequisite as far as I'm concerned. No need to grow the bio when the
> > two features are unlikely to coexist. We can revisit that later should
> > the need arise.
> 
> With NVMe key per I/O support some form of inline encryption and PI are
> very likely to be used together in the not too far future.

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.  IV generation is also unspecified, so it
could be something insecure like always using the same IV.

So effectively the NVMe encryption will be unspecified, untestable, and
unverifiable.  That means that vendors are likely to implement it insecurely,
similar to how they're implementing self-encrypting drives insecurely [1].
(Granted, there are some reasons to think that vendors are less likely to screw
up key per I/O.  But inevitably some will still get it wrong.)

[1] https://www.ieee-security.org/TC/SP2019/papers/310.pdf

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.

And since the way that dm-crypt and fscrypt work is that you select which
algorithm and IV generator you want to use, to even use NVMe "key per I/O" with
them we'd have to add magic settings that say to use some unspecified
hardware-specific encryption format, which could be completely insecure.  As one
of the fscrypt maintainers I'd be really hesistant to accept any such patch, and
I think the dm-crypt people would feel the same way.

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".

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

- Eric