Message ID | 20191003133921.29344-1-ard.biesheuvel@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | crypto: geode-aes - switch to skcipher for cbc(aes) fallback | expand |
Op 03-10-2019 om 15:39 schreef Ard Biesheuvel: > Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated > the generic CBC template wrapper from a blkcipher to a skcipher algo, > to get away from the deprecated blkcipher interface. However, as a side > effect, drivers that instantiate CBC transforms using the blkcipher as > a fallback no longer work, since skciphers can wrap blkciphers but not > the other way around. This broke the geode-aes driver. > > So let's fix it by moving to the sync skcipher interface when allocating > the fallback. > > Cc: Gert Robben <t2@gert.gr> > Cc: Jelle de Jong <jelledejong@powercraft.nl> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Gert, Jelle, > > If you can, please try this patch and report back to the list if it solves > the Geode issue for you. Thanks for the patch! I tried it on Alix 2C2 / Geode LX800 with Linux 5.4-rc1 (also 5.1-5.3 fwiw). At least now openssl doesn't give those errors anymore. (openssl speed -evp aes-128-cbc -elapsed -engine afalg) But looking at the results (<6MB/s), apparently it's not using geode-aes (>30MB/s?). In dmesg can be seen: alg: skcipher: ecb-aes-geode encryption test failed (wrong result) on test vector 1, cfg="out-of-place" alg: skcipher: cbc-aes-geode encryption test failed (wrong result) on test vector 2, cfg="out-of-place" Geode LX AES 0000:00:01.2: GEODE AES engine enabled. In /proc/crypto, drivers cbc-aes-geode/ecb-aes-geode are listed with "selftest: unknown". Driver "geode-aes" has "selftest: passed". I'm happy to test other patches. Regards, Gert
On Thu, 3 Oct 2019 at 23:26, Gert Robben <t2@gert.gr> wrote: > > Op 03-10-2019 om 15:39 schreef Ard Biesheuvel: > > Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated > > the generic CBC template wrapper from a blkcipher to a skcipher algo, > > to get away from the deprecated blkcipher interface. However, as a side > > effect, drivers that instantiate CBC transforms using the blkcipher as > > a fallback no longer work, since skciphers can wrap blkciphers but not > > the other way around. This broke the geode-aes driver. > > > > So let's fix it by moving to the sync skcipher interface when allocating > > the fallback. > > > > Cc: Gert Robben <t2@gert.gr> > > Cc: Jelle de Jong <jelledejong@powercraft.nl> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > Gert, Jelle, > > > > If you can, please try this patch and report back to the list if it solves > > the Geode issue for you. > > Thanks for the patch! > I tried it on Alix 2C2 / Geode LX800 with Linux 5.4-rc1 (also 5.1-5.3 fwiw). > > At least now openssl doesn't give those errors anymore. > (openssl speed -evp aes-128-cbc -elapsed -engine afalg) > But looking at the results (<6MB/s), apparently it's not using geode-aes > (>30MB/s?). > In dmesg can be seen: > > alg: skcipher: ecb-aes-geode encryption test failed (wrong result) on > test vector 1, cfg="out-of-place" > alg: skcipher: cbc-aes-geode encryption test failed (wrong result) on > test vector 2, cfg="out-of-place" > Geode LX AES 0000:00:01.2: GEODE AES engine enabled. > > In /proc/crypto, drivers cbc-aes-geode/ecb-aes-geode are listed with > "selftest: unknown". Driver "geode-aes" has "selftest: passed". > > I'm happy to test other patches. Oops, mistake there on my part Can you replace the two instances of skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); with skcipher_request_set_crypt(req, src, dst, nbytes, desc->info); please?
I'm facing the same problem on one of my VPN gateways. I updated the affected system from Debian Stretch to Buster. Therefore the kernel was updated from 4.9.x to 4.19.x The supplied patch uses some symbols / functions that were introduced with 4.19 (like crypto_sync_skcipher_clear_flags()) so some additional work has to be done for older LTS kernels. Any chance to get a patch working with 4.19? I would be happy to test it. Best regards, Florian > On Thu, 3 Oct 2019 at 23:26, Gert Robben <t2@gert.gr> wrote: >> >> Op 03-10-2019 om 15:39 schreef Ard Biesheuvel: >>> Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated >>> the generic CBC template wrapper from a blkcipher to a skcipher algo, >>> to get away from the deprecated blkcipher interface. However, as a side >>> effect, drivers that instantiate CBC transforms using the blkcipher as >>> a fallback no longer work, since skciphers can wrap blkciphers but not >>> the other way around. This broke the geode-aes driver. >>> >>> So let's fix it by moving to the sync skcipher interface when allocating >>> the fallback. >>> >>> Cc: Gert Robben <t2@gert.gr> >>> Cc: Jelle de Jong <jelledejong@powercraft.nl> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> Gert, Jelle, >>> >>> If you can, please try this patch and report back to the list if it solves >>> the Geode issue for you. >> >> Thanks for the patch! >> I tried it on Alix 2C2 / Geode LX800 with Linux 5.4-rc1 (also 5.1-5.3 fwiw). >> >> At least now openssl doesn't give those errors anymore. >> (openssl speed -evp aes-128-cbc -elapsed -engine afalg) >> But looking at the results (<6MB/s), apparently it's not using geode-aes >> (>30MB/s?). >> In dmesg can be seen: >> >> alg: skcipher: ecb-aes-geode encryption test failed (wrong result) on >> test vector 1, cfg="out-of-place" >> alg: skcipher: cbc-aes-geode encryption test failed (wrong result) on >> test vector 2, cfg="out-of-place" >> Geode LX AES 0000:00:01.2: GEODE AES engine enabled. >> >> In /proc/crypto, drivers cbc-aes-geode/ecb-aes-geode are listed with >> "selftest: unknown". Driver "geode-aes" has "selftest: passed". >> >> I'm happy to test other patches. > > Oops, mistake there on my part > > Can you replace the two instances of > > skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); > > with > > skcipher_request_set_crypt(req, src, dst, nbytes, desc->info); > > please? > >
On Fri, 4 Oct 2019 at 12:21, Florian Bezdeka <florian@bezdeka.de> wrote: > > I'm facing the same problem on one of my VPN gateways. > > I updated the affected system from Debian Stretch to Buster. > Therefore the kernel was updated from 4.9.x to 4.19.x > > The supplied patch uses some symbols / functions that were introduced > with 4.19 (like crypto_sync_skcipher_clear_flags()) so some additional work > has to be done for older LTS kernels. > > Any chance to get a patch working with 4.19? > I would be happy to test it. > Just replace all occurrences of *sync_skcipher* with *skcipher* (including upper case ones), and pass 'CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK' as the third parameter to crypto_alloc_skcipher, then the patch should work with v4.19 as well.
Op 04-10-2019 om 08:16 schreef Ard Biesheuvel: > On Thu, 3 Oct 2019 at 23:26, Gert Robben <t2@gert.gr> wrote: >> Op 03-10-2019 om 15:39 schreef Ard Biesheuvel: >>> Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated >>> the generic CBC template wrapper from a blkcipher to a skcipher algo, >>> to get away from the deprecated blkcipher interface. However, as a side >>> effect, drivers that instantiate CBC transforms using the blkcipher as >>> a fallback no longer work, since skciphers can wrap blkciphers but not >>> the other way around. This broke the geode-aes driver. >>> >>> So let's fix it by moving to the sync skcipher interface when allocating >>> the fallback. >>> >>> Cc: Gert Robben <t2@gert.gr> >>> Cc: Jelle de Jong <jelledejong@powercraft.nl> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> Gert, Jelle, >>> >>> If you can, please try this patch and report back to the list if it solves >>> the Geode issue for you. >> >> Thanks for the patch! >> I tried it on Alix 2C2 / Geode LX800 with Linux 5.4-rc1 (also 5.1-5.3 fwiw). >> >> At least now openssl doesn't give those errors anymore. >> (openssl speed -evp aes-128-cbc -elapsed -engine afalg) >> But looking at the results (<6MB/s), apparently it's not using geode-aes >> (>30MB/s?). >> In dmesg can be seen: >> >> alg: skcipher: ecb-aes-geode encryption test failed (wrong result) on >> test vector 1, cfg="out-of-place" >> alg: skcipher: cbc-aes-geode encryption test failed (wrong result) on >> test vector 2, cfg="out-of-place" >> Geode LX AES 0000:00:01.2: GEODE AES engine enabled. >> >> In /proc/crypto, drivers cbc-aes-geode/ecb-aes-geode are listed with >> "selftest: unknown". Driver "geode-aes" has "selftest: passed". >> >> I'm happy to test other patches. > > Oops, mistake there on my part > > Can you replace the two instances of > > skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); > > with > > skcipher_request_set_crypt(req, src, dst, nbytes, desc->info); > > please? Yes, with that change, now it works in 5.4-rc1: # openssl speed -evp aes-128-cbc -elapsed -engine afalg - - - 8< - - - The 'numbers' are in 1000s of bytes per second processed. type 16 bytes 64 bytes 256 bytes 1024 bytes 8192 bytes 16384 bytes aes-128-cbc 125.63k 499.39k 1858.18k 6377.00k 25753.93k 31167.08k I also quickly tried nginx https, that seems to transfer a file correctly. And a bit faster, but not by this much, I have to look into that further. For now I assume the kernel part seems to be working fine. Thanks, much appreciated! Gert
On Fri, Oct 04, 2019 at 03:29:33PM +0200, Gert Robben wrote: > Op 04-10-2019 om 08:16 schreef Ard Biesheuvel: > > On Thu, 3 Oct 2019 at 23:26, Gert Robben <t2@gert.gr> wrote: > > > Op 03-10-2019 om 15:39 schreef Ard Biesheuvel: > > > > Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated > > > > the generic CBC template wrapper from a blkcipher to a skcipher algo, > > > > to get away from the deprecated blkcipher interface. However, as a side > > > > effect, drivers that instantiate CBC transforms using the blkcipher as > > > > a fallback no longer work, since skciphers can wrap blkciphers but not > > > > the other way around. This broke the geode-aes driver. > > > > > > > > So let's fix it by moving to the sync skcipher interface when allocating > > > > the fallback. > > > > > > > > Cc: Gert Robben <t2@gert.gr> > > > > Cc: Jelle de Jong <jelledejong@powercraft.nl> > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > --- > > > > Gert, Jelle, > > > > > > > > If you can, please try this patch and report back to the list if it solves > > > > the Geode issue for you. > > > > > > Thanks for the patch! > > > I tried it on Alix 2C2 / Geode LX800 with Linux 5.4-rc1 (also 5.1-5.3 fwiw). > > > > > > At least now openssl doesn't give those errors anymore. > > > (openssl speed -evp aes-128-cbc -elapsed -engine afalg) > > > But looking at the results (<6MB/s), apparently it's not using geode-aes > > > (>30MB/s?). > > > In dmesg can be seen: > > > > > > alg: skcipher: ecb-aes-geode encryption test failed (wrong result) on > > > test vector 1, cfg="out-of-place" > > > alg: skcipher: cbc-aes-geode encryption test failed (wrong result) on > > > test vector 2, cfg="out-of-place" > > > Geode LX AES 0000:00:01.2: GEODE AES engine enabled. > > > > > > In /proc/crypto, drivers cbc-aes-geode/ecb-aes-geode are listed with > > > "selftest: unknown". Driver "geode-aes" has "selftest: passed". > > > > > > I'm happy to test other patches. > > > > Oops, mistake there on my part > > > > Can you replace the two instances of > > > > skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); > > > > with > > > > skcipher_request_set_crypt(req, src, dst, nbytes, desc->info); > > > > please? > > Yes, with that change, now it works in 5.4-rc1: > > # openssl speed -evp aes-128-cbc -elapsed -engine afalg > - - - 8< - - - > The 'numbers' are in 1000s of bytes per second processed. > type 16 bytes 64 bytes 256 bytes 1024 bytes 8192 > bytes 16384 bytes > aes-128-cbc 125.63k 499.39k 1858.18k 6377.00k 25753.93k > 31167.08k > > I also quickly tried nginx https, that seems to transfer a file correctly. > And a bit faster, but not by this much, I have to look into that further. > For now I assume the kernel part seems to be working fine. > > Thanks, much appreciated! > Gert Can you check whether it passes the extra self-tests too? I.e. enable CONFIG_CRYPTO_MANAGER_EXTRA_TESTS. - Eric
Op 04-10-2019 om 21:37 schreef Eric Biggers: > On Fri, Oct 04, 2019 at 03:29:33PM +0200, Gert Robben wrote: >> Op 04-10-2019 om 08:16 schreef Ard Biesheuvel: >>> On Thu, 3 Oct 2019 at 23:26, Gert Robben <t2@gert.gr> wrote: >>>> Op 03-10-2019 om 15:39 schreef Ard Biesheuvel: >>>>> Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated >>>>> the generic CBC template wrapper from a blkcipher to a skcipher algo, >>>>> to get away from the deprecated blkcipher interface. However, as a side >>>>> effect, drivers that instantiate CBC transforms using the blkcipher as >>>>> a fallback no longer work, since skciphers can wrap blkciphers but not >>>>> the other way around. This broke the geode-aes driver. >>>>> >>>>> So let's fix it by moving to the sync skcipher interface when allocating >>>>> the fallback. >>>>> >>>>> Cc: Gert Robben <t2@gert.gr> >>>>> Cc: Jelle de Jong <jelledejong@powercraft.nl> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> --- >>>>> Gert, Jelle, >>>>> >>>>> If you can, please try this patch and report back to the list if it solves >>>>> the Geode issue for you. >>>> >>>> Thanks for the patch! >>>> I tried it on Alix 2C2 / Geode LX800 with Linux 5.4-rc1 (also 5.1-5.3 fwiw). >>>> >>>> At least now openssl doesn't give those errors anymore. >>>> (openssl speed -evp aes-128-cbc -elapsed -engine afalg) >>>> But looking at the results (<6MB/s), apparently it's not using geode-aes >>>> (>30MB/s?). >>>> In dmesg can be seen: >>>> >>>> alg: skcipher: ecb-aes-geode encryption test failed (wrong result) on >>>> test vector 1, cfg="out-of-place" >>>> alg: skcipher: cbc-aes-geode encryption test failed (wrong result) on >>>> test vector 2, cfg="out-of-place" >>>> Geode LX AES 0000:00:01.2: GEODE AES engine enabled. >>>> >>>> In /proc/crypto, drivers cbc-aes-geode/ecb-aes-geode are listed with >>>> "selftest: unknown". Driver "geode-aes" has "selftest: passed". >>>> >>>> I'm happy to test other patches. >>> >>> Oops, mistake there on my part >>> >>> Can you replace the two instances of >>> >>> skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); >>> >>> with >>> >>> skcipher_request_set_crypt(req, src, dst, nbytes, desc->info); >>> >>> please? >> >> Yes, with that change, now it works in 5.4-rc1: >> >> # openssl speed -evp aes-128-cbc -elapsed -engine afalg >> - - - 8< - - - >> The 'numbers' are in 1000s of bytes per second processed. >> type 16 bytes 64 bytes 256 bytes 1024 bytes 8192 >> bytes 16384 bytes >> aes-128-cbc 125.63k 499.39k 1858.18k 6377.00k 25753.93k >> 31167.08k >> >> I also quickly tried nginx https, that seems to transfer a file correctly. >> And a bit faster, but not by this much, I have to look into that further. >> For now I assume the kernel part seems to be working fine. >> >> Thanks, much appreciated! >> Gert > > > Can you check whether it passes the extra self-tests too? I.e. enable > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS. Yes, with that option dmesg says additionally (I did several reboots): alg: skcipher: ecb-aes-geode encryption unexpectedly succeeded on test vector "random: len=24 klen=16"; expected_error=-22, cfg="random: inplace use_finup src_divs=[18.14%@alignmask+19, 81.86%@+25] iv_offset=22" alg: skcipher: ecb-aes-geode encryption unexpectedly succeeded on test vector "random: len=148 klen=16"; expected_error=-22, cfg="random: inplace use_digest nosimd src_divs=[100.0%@+22] iv_offset=50" alg: skcipher: ecb-aes-geode encryption unexpectedly succeeded on test vector "random: len=168 klen=16"; expected_error=-22, cfg="random: use_digest nosimd src_divs=[76.11%@alignmask+18, 13.8%@+4056, 4.57%@+3984, 5.36%@+506, 0.68%@+3989, 0.17%@+1620, 0.3%@+4025] iv_offset=27" alg: skcipher: ecb-aes-geode encryption unexpectedly succeeded on test vector "random: len=33 klen=16"; expected_error=-22, cfg="random: may_sleep use_digest src_divs=[97.79%@+20, 2.21%@+4016] iv_offset=38" alg: skcipher: ecb-aes-geode encryption unexpectedly succeeded on test vector "random: len=202 klen=16"; expected_error=-22, cfg="random: inplace use_final nosimd src_divs=[<reimport,nosimd>100.0%@+15] iv_offset=44" alg: skcipher: ecb-aes-geode encryption unexpectedly succeeded on test vector "random: len=60 klen=16"; expected_error=-22, cfg="random: use_digest src_divs=[83.68%@+1899, 7.27%@alignmask+1670, 5.73%@+11, 3.32%@+3985]" Regards, Gert
diff --git a/drivers/crypto/geode-aes.c b/drivers/crypto/geode-aes.c index d81a1297cb9e..b6c47bbc2c49 100644 --- a/drivers/crypto/geode-aes.c +++ b/drivers/crypto/geode-aes.c @@ -10,6 +10,7 @@ #include <linux/spinlock.h> #include <crypto/algapi.h> #include <crypto/aes.h> +#include <crypto/skcipher.h> #include <linux/io.h> #include <linux/delay.h> @@ -166,13 +167,15 @@ static int geode_setkey_blk(struct crypto_tfm *tfm, const u8 *key, /* * The requested key size is not supported by HW, do a fallback */ - op->fallback.blk->base.crt_flags &= ~CRYPTO_TFM_REQ_MASK; - op->fallback.blk->base.crt_flags |= (tfm->crt_flags & CRYPTO_TFM_REQ_MASK); + crypto_sync_skcipher_clear_flags(op->fallback.blk, CRYPTO_TFM_REQ_MASK); + crypto_sync_skcipher_set_flags(op->fallback.blk, + tfm->crt_flags & CRYPTO_TFM_REQ_MASK); - ret = crypto_blkcipher_setkey(op->fallback.blk, key, len); + ret = crypto_sync_skcipher_setkey(op->fallback.blk, key, len); if (ret) { tfm->crt_flags &= ~CRYPTO_TFM_RES_MASK; - tfm->crt_flags |= (op->fallback.blk->base.crt_flags & CRYPTO_TFM_RES_MASK); + tfm->crt_flags |= crypto_sync_skcipher_get_flags(op->fallback.blk) & + CRYPTO_TFM_RES_MASK; } return ret; } @@ -181,33 +184,28 @@ static int fallback_blk_dec(struct blkcipher_desc *desc, struct scatterlist *dst, struct scatterlist *src, unsigned int nbytes) { - unsigned int ret; - struct crypto_blkcipher *tfm; struct geode_aes_op *op = crypto_blkcipher_ctx(desc->tfm); + SYNC_SKCIPHER_REQUEST_ON_STACK(req, op->fallback.blk); - tfm = desc->tfm; - desc->tfm = op->fallback.blk; - - ret = crypto_blkcipher_decrypt_iv(desc, dst, src, nbytes); + skcipher_request_set_sync_tfm(req, op->fallback.blk); + skcipher_request_set_callback(req, 0, NULL, NULL); + skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); - desc->tfm = tfm; - return ret; + return crypto_skcipher_decrypt(req); } + static int fallback_blk_enc(struct blkcipher_desc *desc, struct scatterlist *dst, struct scatterlist *src, unsigned int nbytes) { - unsigned int ret; - struct crypto_blkcipher *tfm; struct geode_aes_op *op = crypto_blkcipher_ctx(desc->tfm); + SYNC_SKCIPHER_REQUEST_ON_STACK(req, op->fallback.blk); - tfm = desc->tfm; - desc->tfm = op->fallback.blk; - - ret = crypto_blkcipher_encrypt_iv(desc, dst, src, nbytes); + skcipher_request_set_sync_tfm(req, op->fallback.blk); + skcipher_request_set_callback(req, 0, NULL, NULL); + skcipher_request_set_crypt(req, dst, src, nbytes, desc->info); - desc->tfm = tfm; - return ret; + return crypto_skcipher_encrypt(req); } static void @@ -366,9 +364,8 @@ static int fallback_init_blk(struct crypto_tfm *tfm) const char *name = crypto_tfm_alg_name(tfm); struct geode_aes_op *op = crypto_tfm_ctx(tfm); - op->fallback.blk = crypto_alloc_blkcipher(name, 0, - CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK); - + op->fallback.blk = crypto_alloc_sync_skcipher(name, 0, + CRYPTO_ALG_NEED_FALLBACK); if (IS_ERR(op->fallback.blk)) { printk(KERN_ERR "Error allocating fallback algo %s\n", name); return PTR_ERR(op->fallback.blk); @@ -381,7 +378,7 @@ static void fallback_exit_blk(struct crypto_tfm *tfm) { struct geode_aes_op *op = crypto_tfm_ctx(tfm); - crypto_free_blkcipher(op->fallback.blk); + crypto_free_sync_skcipher(op->fallback.blk); op->fallback.blk = NULL; } diff --git a/drivers/crypto/geode-aes.h b/drivers/crypto/geode-aes.h index 5c6e131a8f9d..f8a86898ac22 100644 --- a/drivers/crypto/geode-aes.h +++ b/drivers/crypto/geode-aes.h @@ -60,7 +60,7 @@ struct geode_aes_op { u8 *iv; union { - struct crypto_blkcipher *blk; + struct crypto_sync_skcipher *blk; struct crypto_cipher *cip; } fallback; u32 keylen;
Commit 79c65d179a40e145 ("crypto: cbc - Convert to skcipher") updated the generic CBC template wrapper from a blkcipher to a skcipher algo, to get away from the deprecated blkcipher interface. However, as a side effect, drivers that instantiate CBC transforms using the blkcipher as a fallback no longer work, since skciphers can wrap blkciphers but not the other way around. This broke the geode-aes driver. So let's fix it by moving to the sync skcipher interface when allocating the fallback. Cc: Gert Robben <t2@gert.gr> Cc: Jelle de Jong <jelledejong@powercraft.nl> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Gert, Jelle, If you can, please try this patch and report back to the list if it solves the Geode issue for you. -- Ard. drivers/crypto/geode-aes.c | 45 +++++++++----------- drivers/crypto/geode-aes.h | 2 +- 2 files changed, 22 insertions(+), 25 deletions(-) -- 2.20.1