| Message ID | 20201026130450.6947-4-gilad@benyossef.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | crypto: switch to crypto API for EBOIV generation | expand |
On Mon, Oct 26, 2020 at 9:04 PM Milan Broz <gmazyland@gmail.com> wrote: > > > > On 26/10/2020 19:39, Eric Biggers wrote: > > On Mon, Oct 26, 2020 at 07:29:57PM +0100, Milan Broz wrote: > >> On 26/10/2020 18:52, Eric Biggers wrote: > >>> On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote: > >>>> Replace the explicit EBOIV handling in the dm-crypt driver with calls > >>>> into the crypto API, which now possesses the capability to perform > >>>> this processing within the crypto subsystem. > >>>> > >>>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > >>>> > >>>> --- > >>>> drivers/md/Kconfig | 1 + > >>>> drivers/md/dm-crypt.c | 61 ++++++++++++++----------------------------- > >>>> 2 files changed, 20 insertions(+), 42 deletions(-) > >>>> > >>>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig > >>>> index 30ba3573626c..ca6e56a72281 100644 > >>>> --- a/drivers/md/Kconfig > >>>> +++ b/drivers/md/Kconfig > >>>> @@ -273,6 +273,7 @@ config DM_CRYPT > >>>> select CRYPTO > >>>> select CRYPTO_CBC > >>>> select CRYPTO_ESSIV > >>>> + select CRYPTO_EBOIV > >>>> help > >>>> This device-mapper target allows you to create a device that > >>>> transparently encrypts the data on it. You'll need to activate > >>> > >>> Can CRYPTO_EBOIV please not be selected by default? If someone really wants > >>> Bitlocker compatibility support, they can select this option themselves. > >> > >> Please no! Until this move of IV to crypto API, we can rely on > >> support in dm-crypt (if it is not supported, it is just a very old kernel). > >> (Actually, this was the first thing I checked in this patchset - if it is > >> unconditionally enabled for compatibility once dmcrypt is selected.) > >> > >> People already use removable devices with BitLocker. > >> It was the whole point that it works out-of-the-box without enabling anything. > >> > >> If you insist on this to be optional, please better keep this IV inside dmcrypt. > >> (EBOIV has no other use than for disk encryption anyway.) > >> > >> Or maybe another option would be to introduce option under dm-crypt Kconfig that > >> defaults to enabled (like support for foreign/legacy disk encryption schemes) and that > >> selects these IVs/modes. > >> But requiring some random switch in crypto API will only confuse users. > > > > CONFIG_DM_CRYPT can either select every weird combination of algorithms anyone > > can ever be using, or it can select some defaults and require any other needed > > algorithms to be explicitly selected. > > > > In reality, dm-crypt has never even selected any particular block ciphers, even > > AES. Nor has it ever selected XTS. So it's actually always made users (or > > kernel distributors) explicitly select algorithms. Why the Bitlocker support > > suddenly different? > > > > I'd think a lot of dm-crypt users don't want to bloat their kernels with random > > legacy algorithms. > > Yes, but IV is in reality not a cryptographic algorithm, it is kind-of a configuration > "option" of sector encryption mode here. > > We had all of disk-IV inside dmcrypt before - but once it is partially moved into crypto API > (ESSIV, EBOIV for now), it becomes much more complicated for user to select what he needs. > > I think we have no way to check that IV is available from userspace - it > will report the same error as if block cipher is not available, not helping user much > with the error. > > But then I also think we should add abstract dm-crypt options here (Legacy TrueCrypt modes, > Bitlocker modes) that will select these crypto API configuration switches. > Otherwise it will be only a complicated matrix of crypto API options... hm... just thinking out loud, but maybe the right say to go is to not have a build dependency, but add some user assistance code in cryptosetup that parses /proc/crypto after failures to try and suggest the user with a way forward? e.g. if eboiv mapping initiation fails, scan /proc/crypto and either warn of a lack of AES or, assuming some instance of AES is found, warn of lack of EBOIV. It's a little messy and heuristic code for sure, but it lives in a user space utility. Does that sound sane?
On 27/10/2020 07:59, Gilad Ben-Yossef wrote: > On Mon, Oct 26, 2020 at 9:04 PM Milan Broz <gmazyland@gmail.com> wrote: ... >> We had all of disk-IV inside dmcrypt before - but once it is partially moved into crypto API >> (ESSIV, EBOIV for now), it becomes much more complicated for user to select what he needs. >> >> I think we have no way to check that IV is available from userspace - it >> will report the same error as if block cipher is not available, not helping user much >> with the error. >> >> But then I also think we should add abstract dm-crypt options here (Legacy TrueCrypt modes, >> Bitlocker modes) that will select these crypto API configuration switches. >> Otherwise it will be only a complicated matrix of crypto API options... > > hm... just thinking out loud, but maybe the right say to go is to not > have a build dependency, > but add some user assistance code in cryptosetup that parses > /proc/crypto after failures to > try and suggest the user with a way forward? > > e.g. if eboiv mapping initiation fails, scan /proc/crypto and either > warn of a lack of AES > or, assuming some instance of AES is found, warn of lack of EBOIV. > It's a little messy > and heuristic code for sure, but it lives in a user space utility. > > Does that sound sane? Such an idea (try to parse /proc/crypto) is on my TODO list since 2009 :) I expected userspace kernel crypto API could help here, but it seems it is not the case. So yes, I think we need to add something like this in userspace. In combination with the kernel and dmcrypt target version, we could have a pretty good hint matrix for the user, instead of (literally) cryptic errors. (There is also a problem that device-mapper targets are losing detailed error state. We often end just with -EINVAL during table create ... and no descriptive log entry. And leaking info about encrypted devices activation failures to syslog is not a good idea either.) Anyway, this will not fix existing userspace that is not prepared for this kind of EBOIV missing fail, so Herbert's solution seems like the solution for this particular problem for now. (But I agree we should perhaps remove these build dependences in future completely...) Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Oct 26, 2020 at 8:44 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Tue, Oct 27, 2020 at 05:41:55AM +1100, Herbert Xu wrote: > > > > The point is that people rebuilding their kernel can end up with a > > broken system. Just set a default on EBOIV if dm-crypt is on. > > That's not enough as it's an existing option. So we need to > add a new Kconfig option with a default equal to dm-crypt. Sorry if I'm being daft, but what did you refer to be "an existing option"? there was no CONFIG_EBOIV before my patchset, it was simply built as part of dm-crypt so it seems that setting CONFIG_EBOIV default to dm-crypto Kconfig option value does solves the problem, or have I missed something? Thanks, Gilad
On Wed, Oct 28, 2020 at 01:41:28PM +0200, Gilad Ben-Yossef wrote: > > Sorry if I'm being daft, but what did you refer to be "an existing > option"? there was no CONFIG_EBOIV before my patchset, it was simply > built as part of dm-crypt so it seems that setting CONFIG_EBOIV > default to dm-crypto Kconfig option value does solves the problem, or > have I missed something? Oh I'm mistaken then. I thought it was an existing option. If it's a new option then a default depending on dm-crypt should be sufficient. Thanks,
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig index 30ba3573626c..ca6e56a72281 100644 --- a/drivers/md/Kconfig +++ b/drivers/md/Kconfig @@ -273,6 +273,7 @@ config DM_CRYPT select CRYPTO select CRYPTO_CBC select CRYPTO_ESSIV + select CRYPTO_EBOIV help This device-mapper target allows you to create a device that transparently encrypts the data on it. You'll need to activate diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 148960721254..cad8f4e3f5d9 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -716,47 +716,18 @@ static int crypt_iv_random_gen(struct crypt_config *cc, u8 *iv, return 0; } -static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target *ti, - const char *opts) -{ - if (crypt_integrity_aead(cc)) { - ti->error = "AEAD transforms not supported for EBOIV"; - return -EINVAL; - } - - if (crypto_skcipher_blocksize(any_tfm(cc)) != cc->iv_size) { - ti->error = "Block size of EBOIV cipher does " - "not match IV size of block cipher"; - return -EINVAL; - } - - return 0; -} - static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv, struct dm_crypt_request *dmreq) { - u8 buf[MAX_CIPHER_BLOCKSIZE] __aligned(__alignof__(__le64)); - struct skcipher_request *req; - struct scatterlist src, dst; - struct crypto_wait wait; - int err; - - req = skcipher_request_alloc(any_tfm(cc), GFP_NOIO); - if (!req) - return -ENOMEM; - - memset(buf, 0, cc->iv_size); - *(__le64 *)buf = cpu_to_le64(dmreq->iv_sector * cc->sector_size); - sg_init_one(&src, page_address(ZERO_PAGE(0)), cc->iv_size); - sg_init_one(&dst, iv, cc->iv_size); - skcipher_request_set_crypt(req, &src, &dst, cc->iv_size, buf); - skcipher_request_set_callback(req, 0, crypto_req_done, &wait); - err = crypto_wait_req(crypto_skcipher_encrypt(req), &wait); - skcipher_request_free(req); + /* + * ESSIV encryption of the IV is handled by the crypto API, + * so compute and pass the sector offset here. + */ + memset(iv, 0, cc->iv_size); + *(__le64 *)iv = cpu_to_le64(dmreq->iv_sector * cc->sector_size); - return err; + return 0; } static void crypt_iv_elephant_dtr(struct crypt_config *cc) @@ -777,13 +748,9 @@ static int crypt_iv_elephant_ctr(struct crypt_config *cc, struct dm_target *ti, if (IS_ERR(elephant->tfm)) { r = PTR_ERR(elephant->tfm); elephant->tfm = NULL; - return r; } - r = crypt_iv_eboiv_ctr(cc, ti, NULL); - if (r) - crypt_iv_elephant_dtr(cc); - return r; + return 0; } static void diffuser_disk_to_cpu(u32 *d, size_t n) @@ -1092,7 +1059,6 @@ static struct crypt_iv_operations crypt_iv_random_ops = { }; static struct crypt_iv_operations crypt_iv_eboiv_ops = { - .ctr = crypt_iv_eboiv_ctr, .generator = crypt_iv_eboiv_gen }; @@ -2739,6 +2705,15 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key cipher_api = buf; } + if (*ivmode && (!strcmp(*ivmode, "eboiv") || !strcmp(*ivmode, "elephant"))) { + ret = snprintf(buf, CRYPTO_MAX_ALG_NAME, "eboiv(%s)", cipher_api); + if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME) { + ti->error = "Cannot allocate cipher string"; + return -ENOMEM; + } + cipher_api = buf; + } + cc->key_parts = cc->tfms_count; /* Allocate cipher */ @@ -2817,6 +2792,8 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key } ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, "essiv(%s(%s),%s)", chainmode, cipher, *ivopts); + } else if (*ivmode && (!strcmp(*ivmode, "eboiv") || !strcmp(*ivmode, "elephant"))) { + ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, "eboiv(%s(%s))", chainmode, cipher); } else { ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, "%s(%s)", chainmode, cipher);
Replace the explicit EBOIV handling in the dm-crypt driver with calls into the crypto API, which now possesses the capability to perform this processing within the crypto subsystem. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> --- drivers/md/Kconfig | 1 + drivers/md/dm-crypt.c | 61 ++++++++++++++----------------------------- 2 files changed, 20 insertions(+), 42 deletions(-)