Message-ID: <20240921185519.GA2187@quark.localdomain>
Date: Sat, 21 Sep 2024 11:55:19 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Md Sadre Alam <quic_mdalam@...cinc.com>
Cc: axboe@...nel.dk, song@...nel.org, yukuai3@...wei.com, agk@...hat.com,
	snitzer@...nel.org, mpatocka@...hat.com, adrian.hunter@...el.com,
	quic_asutoshd@...cinc.com, ritesh.list@...il.com,
	ulf.hansson@...aro.org, andersson@...nel.org,
	konradybcio@...nel.org, kees@...nel.org, gustavoars@...nel.org,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-raid@...r.kernel.org, dm-devel@...ts.linux.dev,
	linux-mmc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	linux-hardening@...r.kernel.org, quic_srichara@...cinc.com,
	quic_varada@...cinc.com
Subject: Re: [PATCH v2 1/3] dm-inlinecrypt: Add inline encryption support

Hi,

On Mon, Sep 16, 2024 at 02:27:39PM +0530, Md Sadre Alam wrote:
> QCOM SDCC controller supports Inline Crypto Engine
> This driver will enables inline encryption/decryption
> for ICE. The algorithm supported by ICE are XTS(AES)
> and CBC(AES).
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
> ---
> 
> Change in [v2]
> 
> * Added dm-inlinecrypt driver support
> 
> * squash the patch blk-crypto: Add additional algo modes for Inline
>   encryption and md: dm-crypt: Add additional algo modes for inline
>   encryption and added in this
> 
> Change in [v1]
> 
> * This patch was not included in [v1]
> 
>  block/blk-crypto.c           |  21 +++
>  drivers/md/Kconfig           |   8 +
>  drivers/md/Makefile          |   1 +
>  drivers/md/dm-inline-crypt.c | 316 +++++++++++++++++++++++++++++++++++
>  include/linux/blk-crypto.h   |   3 +
>  5 files changed, 349 insertions(+)
>  create mode 100644 drivers/md/dm-inline-crypt.c

Thanks for working on this!  Android uses a similar device-mapper target called
dm-default-key
(https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c),
and I've been looking for the best way to get the functionality upstream.  The
main challenge is that dm-default-key is integrated with fscrypt, such that if
fscrypt encrypts the data, then the data isn't also encrypted with the block
device key.  There are also cases such as f2fs garbage collection in which
filesystems read/write raw data without en/decryption by any key.  So
essentially a passthrough mode is supported on individual I/O requests.

It looks like this patch not only does not support that, but it ignores the
existence of fscrypt (or any other use of inline encryption by filesystems)
entirely, and overrides any filesystem-provided key with the block device's.  At
the very least, this case would need to be explicitly not supported initially,
i.e. dm-inlinecrypt would error out if the upper layer already provided a key.

But I would like there to be an agreed-upon way to extend the code to support
the pass-through mode, so that filesystem and block device level encryption work
properly together.  It can indeed be done with a device-mapper target (as
Android does already), whether it's called "dm-inlinecrypt" or "dm-default-key",
but not in a standalone way: pass-through requests also require an addition to
struct bio and some support in block and filesystem code.  Previously, people
have said that supporting this functionality natively in the block layer would
be a better fit than a dm target (e.g., see the thread
https://lore.kernel.org/all/1658316391-13472-1-git-send-email-israelr@nvidia.com/T/#u).
I'd appreciate people's feedback on which approach they'd prefer.

Anyway, assuming the device-mapper target approach, I also have some other
feedback on this patch:

New algorithms should not be added in the same patch as a new dm target.  Also,
I do not see why there is any need for the new algorithms you are adding
(AES-128-XTS, AES-128-CBC, and AES-256-CBC).  XTS is preferable to CBC, and
AES-256 is preferable to AES-128.  AES-256-XTS is already supported, both by
blk-crypto and by Qualcomm ICE.  So you should just use AES-256-XTS.

There are also a lot of miscellaneous issues with the proposed code.  Missing
->io_hints and ->status methods, truncating IVs to 32 bits, unnecessarily using
a custom algorithm name syntax that doesn't make the IV generation method
explicit, unnecessarily splitting bios, incrementing IVs every 512 bytes instead
of each sector, etc.  I can comment on all of these in detail if you want, but
to start it might be helpful to just check out dm-default-key
(https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c)
and maybe base your code on that, as it has handled these issues already.  E.g.,
its syntax is aligned with dm-crypt's, it implements ->io_hints and ->status,
and it calculates the maximum DUN correctly so that it can be determined
correctly whether the inline encryption hardware can be used or not.

Finally, the commit message needs to summarize what the patch does and what its
motivation is.  Currently it just talks about the Qualcomm ICE driver and
doesn't actually say anything about dm-inlinecrypt.  Yes, the motivation for
dm-inlinecrypt involves being able to take advantage of inline encryption
hardware such as Qualcomm ICE, but it's not explained.

Thanks,

- Eric

Powered by blists - more mailing lists