* [PATCH] crypto: Fix next IV issue for CTS template @ 2017-02-17 3:47 Libo.Wang 2017-02-17 7:12 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Libo.Wang @ 2017-02-17 3:47 UTC (permalink / raw) To: herbert, davem, linux-crypto Cc: ard.biesheuvel, Ofir.Drang, Libo Wang, Dennis Chen From: Libo Wang <Libo Wang@arm.com> CTS template assumes underlying CBC algorithm will carry out next IV for further process.But some implementations of CBC algorithm in kernel break this assumption, for example, some hardware crypto drivers ignore next IV for performance consider, inthis case, tcry cts(cbc(aes)) test case will fail. This patch is trying to fix it by getting next IV information ready before last two blocks processed. Signed-off-by: Libo Wang <libo.wang@arm.com> Signed-off-by: Dennis Chen <dennis.chen@arm.com> --- crypto/cts.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/crypto/cts.c b/crypto/cts.c index a1335d6..712164b 100644 --- a/crypto/cts.c +++ b/crypto/cts.c @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req) unsigned int nbytes = req->cryptlen; int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; unsigned int offset; + int ret = 0; skcipher_request_set_tfm(subreq, ctx->child); @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req) skcipher_request_set_crypt(subreq, req->src, req->dst, offset, req->iv); - return crypto_skcipher_encrypt(subreq) ?: - cts_cbc_encrypt(req); + /* process CBC blocks */ + ret = crypto_skcipher_encrypt(subreq); + /* process last two blocks */ + if (!ret) { + /* Get IVn-1 back */ + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0); + /* Continue last two blocks */ + return cts_cbc_encrypt(req); + } + + return ret; } static int cts_cbc_decrypt(struct skcipher_request *req) @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req) int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; unsigned int offset; u8 *space; + int ret = 0; + u8 iv_next[bsize]; skcipher_request_set_tfm(subreq, ctx->child); @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req) skcipher_request_set_crypt(subreq, req->src, req->dst, offset, req->iv); - return crypto_skcipher_decrypt(subreq) ?: - cts_cbc_decrypt(req); + /* process last two blocks */ + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0); + ret = crypto_skcipher_decrypt(subreq); + if (!ret) { + /* Set Next IV */ + subreq->iv = iv_next; + /* process last two blocks */ + return cts_cbc_decrypt(req); + } + + return ret; } static int crypto_cts_init_tfm(struct crypto_skcipher *tfm) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: Fix next IV issue for CTS template 2017-02-17 3:47 [PATCH] crypto: Fix next IV issue for CTS template Libo.Wang @ 2017-02-17 7:12 ` Ard Biesheuvel 2017-02-17 9:17 ` Dennis Chen 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2017-02-17 7:12 UTC (permalink / raw) To: Libo.Wang Cc: Herbert Xu, David S. Miller, linux-crypto@vger.kernel.org, Ofir.Drang, Dennis Chen Hello Libo, On 17 February 2017 at 03:47, <Libo.Wang@arm.com> wrote: > From: Libo Wang <Libo Wang@arm.com> > > CTS template assumes underlying CBC algorithm will carry out next IV for > further process.But some implementations of CBC algorithm in kernel break > this assumption, for example, some hardware crypto drivers ignore next IV > for performance consider, inthis case, tcry cts(cbc(aes)) test case will > fail. This patch is trying to fix it by getting next IV information ready > before last two blocks processed. > > Signed-off-by: Libo Wang <libo.wang@arm.com> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> Which algorithms in particular break this assumption? I recently fixed some ARM accelerated software drivers for this reason. If there are others, we should fix those rather than try to fix it in the CTS driver. > --- > crypto/cts.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/crypto/cts.c b/crypto/cts.c > index a1335d6..712164b 100644 > --- a/crypto/cts.c > +++ b/crypto/cts.c > @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req) > unsigned int nbytes = req->cryptlen; > int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; > unsigned int offset; > + int ret = 0; > > skcipher_request_set_tfm(subreq, ctx->child); > > @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req) > skcipher_request_set_crypt(subreq, req->src, req->dst, > offset, req->iv); > > - return crypto_skcipher_encrypt(subreq) ?: > - cts_cbc_encrypt(req); > + /* process CBC blocks */ > + ret = crypto_skcipher_encrypt(subreq); > + /* process last two blocks */ > + if (!ret) { What happens if an async driver returns -EINPROGRESS here? > + /* Get IVn-1 back */ > + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0); > + /* Continue last two blocks */ > + return cts_cbc_encrypt(req); > + } > + > + return ret; > } > > static int cts_cbc_decrypt(struct skcipher_request *req) > @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req) > int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; > unsigned int offset; > u8 *space; > + int ret = 0; > + u8 iv_next[bsize]; > > skcipher_request_set_tfm(subreq, ctx->child); > > @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req) > skcipher_request_set_crypt(subreq, req->src, req->dst, > offset, req->iv); > > - return crypto_skcipher_decrypt(subreq) ?: > - cts_cbc_decrypt(req); > + /* process last two blocks */ > + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0); > + ret = crypto_skcipher_decrypt(subreq); > + if (!ret) { > + /* Set Next IV */ > + subreq->iv = iv_next; > + /* process last two blocks */ > + return cts_cbc_decrypt(req); > + } > + > + return ret; > } > > static int crypto_cts_init_tfm(struct crypto_skcipher *tfm) > -- > 1.8.3.1 > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. Please configure your email client so it doesn't spit out these. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: Fix next IV issue for CTS template 2017-02-17 7:12 ` Ard Biesheuvel @ 2017-02-17 9:17 ` Dennis Chen 2017-02-17 9:23 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Dennis Chen @ 2017-02-17 9:17 UTC (permalink / raw) To: Ard Biesheuvel Cc: Libo.Wang, Herbert Xu, David S. Miller, linux-crypto@vger.kernel.org, Ofir.Drang, nd Hello Ard, Morning! On Fri, Feb 17, 2017 at 07:12:46AM +0000, Ard Biesheuvel wrote: > Hello Libo, > > On 17 February 2017 at 03:47, <Libo.Wang@arm.com> wrote: > > From: Libo Wang <Libo Wang@arm.com> > > > > CTS template assumes underlying CBC algorithm will carry out next IV for > > further process.But some implementations of CBC algorithm in kernel break > > this assumption, for example, some hardware crypto drivers ignore next IV > > for performance consider, inthis case, tcry cts(cbc(aes)) test case will > > fail. This patch is trying to fix it by getting next IV information ready > > before last two blocks processed. > > > > Signed-off-by: Libo Wang <libo.wang@arm.com> > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > > Which algorithms in particular break this assumption? I recently fixed > some ARM accelerated software drivers for this reason. If there are > others, we should fix those rather than try to fix it in the CTS > driver. > There're some ARM HW accelerated drivers which are not upstream yet(IP license) breaks this assumption. The current CTS template requires the underlying CBC algorithm always fill the next IV, but in some cases like CBC standalone mode, it doesn't need to fill the next IV, so apparently it will induce performance degrade from the point of drivers, that's why we fix it in the CTS template,except that it will also mitigate the potential defect of other HW-based CBC drivers. > > > --- > > crypto/cts.c | 29 +++++++++++++++++++++++++---- > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > diff --git a/crypto/cts.c b/crypto/cts.c > > index a1335d6..712164b 100644 > > --- a/crypto/cts.c > > +++ b/crypto/cts.c > > @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req) > > unsigned int nbytes = req->cryptlen; > > int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; > > unsigned int offset; > > + int ret = 0; > > > > skcipher_request_set_tfm(subreq, ctx->child); > > > > @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req) > > skcipher_request_set_crypt(subreq, req->src, req->dst, > > offset, req->iv); > > > > - return crypto_skcipher_encrypt(subreq) ?: > > - cts_cbc_encrypt(req); > > + /* process CBC blocks */ > > + ret = crypto_skcipher_encrypt(subreq); > > + /* process last two blocks */ > > + if (!ret) { > > What happens if an async driver returns -EINPROGRESS here? For this case, the CTS will return -EINPROGRESS directly. Thanks, Dennis > > > + /* Get IVn-1 back */ > > + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0); > > + /* Continue last two blocks */ > > + return cts_cbc_encrypt(req); > > + } > > + > > + return ret; > > } > > > > static int cts_cbc_decrypt(struct skcipher_request *req) > > @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req) > > int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; > > unsigned int offset; > > u8 *space; > > + int ret = 0; > > + u8 iv_next[bsize]; > > > > skcipher_request_set_tfm(subreq, ctx->child); > > > > @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req) > > skcipher_request_set_crypt(subreq, req->src, req->dst, > > offset, req->iv); > > > > - return crypto_skcipher_decrypt(subreq) ?: > > - cts_cbc_decrypt(req); > > + /* process last two blocks */ > > + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0); > > + ret = crypto_skcipher_decrypt(subreq); > > + if (!ret) { > > + /* Set Next IV */ > > + subreq->iv = iv_next; > > + /* process last two blocks */ > > + return cts_cbc_decrypt(req); > > + } > > + > > + return ret; > > } > > > > static int crypto_cts_init_tfm(struct crypto_skcipher *tfm) > > -- > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: Fix next IV issue for CTS template 2017-02-17 9:17 ` Dennis Chen @ 2017-02-17 9:23 ` Ard Biesheuvel 2017-02-17 10:06 ` Dennis Chen 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2017-02-17 9:23 UTC (permalink / raw) To: Dennis Chen Cc: Libo.Wang, Herbert Xu, David S. Miller, linux-crypto@vger.kernel.org, Ofir.Drang, nd > On 17 Feb 2017, at 09:17, Dennis Chen <dennis.chen@arm.com> wrote: > > Hello Ard, > Morning! >> On Fri, Feb 17, 2017 at 07:12:46AM +0000, Ard Biesheuvel wrote: >> Hello Libo, >> >>> On 17 February 2017 at 03:47, <Libo.Wang@arm.com> wrote: >>> From: Libo Wang <Libo Wang@arm.com> >>> >>> CTS template assumes underlying CBC algorithm will carry out next IV for >>> further process.But some implementations of CBC algorithm in kernel break >>> this assumption, for example, some hardware crypto drivers ignore next IV >>> for performance consider, inthis case, tcry cts(cbc(aes)) test case will >>> fail. This patch is trying to fix it by getting next IV information ready >>> before last two blocks processed. >>> >>> Signed-off-by: Libo Wang <libo.wang@arm.com> >>> Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> >> Which algorithms in particular break this assumption? I recently fixed >> some ARM accelerated software drivers for this reason. If there are >> others, we should fix those rather than try to fix it in the CTS >> driver. >> > There're some ARM HW accelerated drivers which are not upstream yet(IP license) > breaks this assumption. The current CTS template requires the underlying CBC > algorithm always fill the next IV, but in some cases like CBC standalone mode, > it doesn't need to fill the next IV, so apparently it will induce performance > degrade from the point of drivers, that's why we fix it in the CTS template,except > that it will also mitigate the potential defect of other HW-based CBC drivers. You should fix this in the driver, not in the cts code. >> >>> --- >>> crypto/cts.c | 29 +++++++++++++++++++++++++---- >>> 1 file changed, 25 insertions(+), 4 deletions(-) >>> >>> diff --git a/crypto/cts.c b/crypto/cts.c >>> index a1335d6..712164b 100644 >>> --- a/crypto/cts.c >>> +++ b/crypto/cts.c >>> @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req) >>> unsigned int nbytes = req->cryptlen; >>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; >>> unsigned int offset; >>> + int ret = 0; >>> >>> skcipher_request_set_tfm(subreq, ctx->child); >>> >>> @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req) >>> skcipher_request_set_crypt(subreq, req->src, req->dst, >>> offset, req->iv); >>> >>> - return crypto_skcipher_encrypt(subreq) ?: >>> - cts_cbc_encrypt(req); >>> + /* process CBC blocks */ >>> + ret = crypto_skcipher_encrypt(subreq); >>> + /* process last two blocks */ >>> + if (!ret) { >> >> What happens if an async driver returns -EINPROGRESS here? > For this case, the CTS will return -EINPROGRESS directly. > I see. But how do you handle the missing out IV when the async call completes? >> >>> + /* Get IVn-1 back */ >>> + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0); >>> + /* Continue last two blocks */ >>> + return cts_cbc_encrypt(req); >>> + } >>> + >>> + return ret; >>> } >>> >>> static int cts_cbc_decrypt(struct skcipher_request *req) >>> @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req) >>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; >>> unsigned int offset; >>> u8 *space; >>> + int ret = 0; >>> + u8 iv_next[bsize]; >>> >>> skcipher_request_set_tfm(subreq, ctx->child); >>> >>> @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req) >>> skcipher_request_set_crypt(subreq, req->src, req->dst, >>> offset, req->iv); >>> >>> - return crypto_skcipher_decrypt(subreq) ?: >>> - cts_cbc_decrypt(req); >>> + /* process last two blocks */ >>> + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0); >>> + ret = crypto_skcipher_decrypt(subreq); >>> + if (!ret) { >>> + /* Set Next IV */ >>> + subreq->iv = iv_next; >>> + /* process last two blocks */ >>> + return cts_cbc_decrypt(req); >>> + } >>> + >>> + return ret; >>> } >>> >>> static int crypto_cts_init_tfm(struct crypto_skcipher *tfm) >>> -- >>> 1.8.3.1 >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: Fix next IV issue for CTS template 2017-02-17 9:23 ` Ard Biesheuvel @ 2017-02-17 10:06 ` Dennis Chen 2017-02-17 10:42 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Dennis Chen @ 2017-02-17 10:06 UTC (permalink / raw) To: Ard Biesheuvel Cc: Libo.Wang, Herbert Xu, David S. Miller, linux-crypto@vger.kernel.org, Ofir.Drang, nd On Fri, Feb 17, 2017 at 09:23:00AM +0000, Ard Biesheuvel wrote: > > > On 17 Feb 2017, at 09:17, Dennis Chen <dennis.chen@arm.com> wrote: > > > > Hello Ard, > > Morning! > >> On Fri, Feb 17, 2017 at 07:12:46AM +0000, Ard Biesheuvel wrote: > >> Hello Libo, > >> > >>> On 17 February 2017 at 03:47, <Libo.Wang@arm.com> wrote: > >>> From: Libo Wang <Libo Wang@arm.com> > >>> > >>> CTS template assumes underlying CBC algorithm will carry out next IV for > >>> further process.But some implementations of CBC algorithm in kernel break > >>> this assumption, for example, some hardware crypto drivers ignore next IV > >>> for performance consider, inthis case, tcry cts(cbc(aes)) test case will > >>> fail. This patch is trying to fix it by getting next IV information ready > >>> before last two blocks processed. > >>> > >>> Signed-off-by: Libo Wang <libo.wang@arm.com> > >>> Signed-off-by: Dennis Chen <dennis.chen@arm.com> > >> > >> Which algorithms in particular break this assumption? I recently fixed > >> some ARM accelerated software drivers for this reason. If there are > >> others, we should fix those rather than try to fix it in the CTS > >> driver. > >> > > There're some ARM HW accelerated drivers which are not upstream yet(IP license) > > breaks this assumption. The current CTS template requires the underlying CBC > > algorithm always fill the next IV, but in some cases like CBC standalone mode, > > it doesn't need to fill the next IV, so apparently it will induce performance > > degrade from the point of drivers, that's why we fix it in the CTS template,except > > that it will also mitigate the potential defect of other HW-based CBC drivers. > > You should fix this in the driver, not in the cts code. > Ah, we're open for the change, but is it better to give the reason to fix it in the driver instead of in cts code? > >> > >>> --- > >>> crypto/cts.c | 29 +++++++++++++++++++++++++---- > >>> 1 file changed, 25 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/crypto/cts.c b/crypto/cts.c > >>> index a1335d6..712164b 100644 > >>> --- a/crypto/cts.c > >>> +++ b/crypto/cts.c > >>> @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req) > >>> unsigned int nbytes = req->cryptlen; > >>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; > >>> unsigned int offset; > >>> + int ret = 0; > >>> > >>> skcipher_request_set_tfm(subreq, ctx->child); > >>> > >>> @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req) > >>> skcipher_request_set_crypt(subreq, req->src, req->dst, > >>> offset, req->iv); > >>> > >>> - return crypto_skcipher_encrypt(subreq) ?: > >>> - cts_cbc_encrypt(req); > >>> + /* process CBC blocks */ > >>> + ret = crypto_skcipher_encrypt(subreq); > >>> + /* process last two blocks */ > >>> + if (!ret) { > >> > >> What happens if an async driver returns -EINPROGRESS here? > > For this case, the CTS will return -EINPROGRESS directly. > > > > I see. But how do you handle the missing out IV when the async call completes? > Not quite understand here, is there some logic inconsistent of the change here comparing the original one to handle this case? We think it will have the same result if the fix is in driver layer. Thanks, Dennis > > >> > >>> + /* Get IVn-1 back */ > >>> + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0); > >>> + /* Continue last two blocks */ > >>> + return cts_cbc_encrypt(req); > >>> + } > >>> + > >>> + return ret; > >>> } > >>> > >>> static int cts_cbc_decrypt(struct skcipher_request *req) > >>> @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req) > >>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; > >>> unsigned int offset; > >>> u8 *space; > >>> + int ret = 0; > >>> + u8 iv_next[bsize]; > >>> > >>> skcipher_request_set_tfm(subreq, ctx->child); > >>> > >>> @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req) > >>> skcipher_request_set_crypt(subreq, req->src, req->dst, > >>> offset, req->iv); > >>> > >>> - return crypto_skcipher_decrypt(subreq) ?: > >>> - cts_cbc_decrypt(req); > >>> + /* process last two blocks */ > >>> + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0); > >>> + ret = crypto_skcipher_decrypt(subreq); > >>> + if (!ret) { > >>> + /* Set Next IV */ > >>> + subreq->iv = iv_next; > >>> + /* process last two blocks */ > >>> + return cts_cbc_decrypt(req); > >>> + } > >>> + > >>> + return ret; > >>> } > >>> > >>> static int crypto_cts_init_tfm(struct crypto_skcipher *tfm) > >>> -- > >>> 1.8.3.1 > >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: Fix next IV issue for CTS template 2017-02-17 10:06 ` Dennis Chen @ 2017-02-17 10:42 ` Ard Biesheuvel 0 siblings, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2017-02-17 10:42 UTC (permalink / raw) To: Dennis Chen Cc: Libo.Wang, Herbert Xu, David S. Miller, linux-crypto@vger.kernel.org, Ofir.Drang, nd > On 17 Feb 2017, at 10:06, Dennis Chen <dennis.chen@arm.com> wrote: > >> On Fri, Feb 17, 2017 at 09:23:00AM +0000, Ard Biesheuvel wrote: >> >>> On 17 Feb 2017, at 09:17, Dennis Chen <dennis.chen@arm.com> wrote: >>> >>> Hello Ard, >>> Morning! >>>> On Fri, Feb 17, 2017 at 07:12:46AM +0000, Ard Biesheuvel wrote: >>>> Hello Libo, >>>> >>>>> On 17 February 2017 at 03:47, <Libo.Wang@arm.com> wrote: >>>>> From: Libo Wang <Libo Wang@arm.com> >>>>> >>>>> CTS template assumes underlying CBC algorithm will carry out next IV for >>>>> further process.But some implementations of CBC algorithm in kernel break >>>>> this assumption, for example, some hardware crypto drivers ignore next IV >>>>> for performance consider, inthis case, tcry cts(cbc(aes)) test case will >>>>> fail. This patch is trying to fix it by getting next IV information ready >>>>> before last two blocks processed. >>>>> >>>>> Signed-off-by: Libo Wang <libo.wang@arm.com> >>>>> Signed-off-by: Dennis Chen <dennis.chen@arm.com> >>>> >>>> Which algorithms in particular break this assumption? I recently fixed >>>> some ARM accelerated software drivers for this reason. If there are >>>> others, we should fix those rather than try to fix it in the CTS >>>> driver. >>>> >>> There're some ARM HW accelerated drivers which are not upstream yet(IP license) >>> breaks this assumption. The current CTS template requires the underlying CBC >>> algorithm always fill the next IV, but in some cases like CBC standalone mode, >>> it doesn't need to fill the next IV, so apparently it will induce performance >>> degrade from the point of drivers, that's why we fix it in the CTS template,except >>> that it will also mitigate the potential defect of other HW-based CBC drivers. >> >> You should fix this in the driver, not in the cts code. >> > Ah, we're open for the change, but is it better to give the reason to fix it in the driver > instead of in cts code? Because it is the h/w driver that violates the api. Please search the list: I discussed this with Herbert very recently >>>> >>>>> --- >>>>> crypto/cts.c | 29 +++++++++++++++++++++++++---- >>>>> 1 file changed, 25 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/crypto/cts.c b/crypto/cts.c >>>>> index a1335d6..712164b 100644 >>>>> --- a/crypto/cts.c >>>>> +++ b/crypto/cts.c >>>>> @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req) >>>>> unsigned int nbytes = req->cryptlen; >>>>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; >>>>> unsigned int offset; >>>>> + int ret = 0; >>>>> >>>>> skcipher_request_set_tfm(subreq, ctx->child); >>>>> >>>>> @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req) >>>>> skcipher_request_set_crypt(subreq, req->src, req->dst, >>>>> offset, req->iv); >>>>> >>>>> - return crypto_skcipher_encrypt(subreq) ?: >>>>> - cts_cbc_encrypt(req); >>>>> + /* process CBC blocks */ >>>>> + ret = crypto_skcipher_encrypt(subreq); >>>>> + /* process last two blocks */ >>>>> + if (!ret) { >>>> >>>> What happens if an async driver returns -EINPROGRESS here? >>> For this case, the CTS will return -EINPROGRESS directly. >>> >> >> I see. But how do you handle the missing out IV when the async call completes? >> > Not quite understand here, is there some logic inconsistent of the change here comparing the > original one to handle this case? We think it will have the same result if the fix is in driver layer. > > Thanks, > Dennis >> >>>> >>>>> + /* Get IVn-1 back */ >>>>> + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0); >>>>> + /* Continue last two blocks */ >>>>> + return cts_cbc_encrypt(req); >>>>> + } >>>>> + >>>>> + return ret; >>>>> } >>>>> >>>>> static int cts_cbc_decrypt(struct skcipher_request *req) >>>>> @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req) >>>>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1; >>>>> unsigned int offset; >>>>> u8 *space; >>>>> + int ret = 0; >>>>> + u8 iv_next[bsize]; >>>>> >>>>> skcipher_request_set_tfm(subreq, ctx->child); >>>>> >>>>> @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req) >>>>> skcipher_request_set_crypt(subreq, req->src, req->dst, >>>>> offset, req->iv); >>>>> >>>>> - return crypto_skcipher_decrypt(subreq) ?: >>>>> - cts_cbc_decrypt(req); >>>>> + /* process last two blocks */ >>>>> + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0); >>>>> + ret = crypto_skcipher_decrypt(subreq); >>>>> + if (!ret) { >>>>> + /* Set Next IV */ >>>>> + subreq->iv = iv_next; >>>>> + /* process last two blocks */ >>>>> + return cts_cbc_decrypt(req); >>>>> + } >>>>> + >>>>> + return ret; >>>>> } >>>>> >>>>> static int crypto_cts_init_tfm(struct crypto_skcipher *tfm) >>>>> -- >>>>> 1.8.3.1 >>>>> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-17 10:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-17 3:47 [PATCH] crypto: Fix next IV issue for CTS template Libo.Wang 2017-02-17 7:12 ` Ard Biesheuvel 2017-02-17 9:17 ` Dennis Chen 2017-02-17 9:23 ` Ard Biesheuvel 2017-02-17 10:06 ` Dennis Chen 2017-02-17 10:42 ` Ard Biesheuvel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.