| Message ID | 20190819142226.1703-3-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drivers/crypto - s5p fixes | expand |
On 19.08.2019 16:22, Ard Biesheuvel wrote: > Align the s5p ctr(aes) implementation with other implementations > of the same mode, by setting the block size to 1. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/crypto/s5p-sss.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c > index ef90c58edb1f..010f1bb20dad 100644 > --- a/drivers/crypto/s5p-sss.c > +++ b/drivers/crypto/s5p-sss.c > @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { > .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | > CRYPTO_ALG_ASYNC | > CRYPTO_ALG_KERN_DRIVER_ONLY, > - .cra_blocksize = AES_BLOCK_SIZE, > + .cra_blocksize = 1, > .cra_ctxsize = sizeof(struct s5p_aes_ctx), > .cra_alignmask = 0x0f, > .cra_type = &crypto_ablkcipher_type, > Acked-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Align the s5p ctr(aes) implementation with other implementations > of the same mode, by setting the block size to 1. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/crypto/s5p-sss.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c > index ef90c58edb1f..010f1bb20dad 100644 > --- a/drivers/crypto/s5p-sss.c > +++ b/drivers/crypto/s5p-sss.c > @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { > .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | > CRYPTO_ALG_ASYNC | > CRYPTO_ALG_KERN_DRIVER_ONLY, > - .cra_blocksize = AES_BLOCK_SIZE, > + .cra_blocksize = 1, This makes sense but I wonder how does it work later with s5p_aes_crypt() and its check for request length alignment (AES_BLOCK_SIZE). With block size of 1 byte, I understand that req->nbytes can be for example 4 bytes which is not AES block aligned... If my reasoning is correct, then the CTR mode in s5p-sss is not fully working. Best regards, Krzysztof
On Tue, 20 Aug 2019 at 13:24, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > > Align the s5p ctr(aes) implementation with other implementations > > of the same mode, by setting the block size to 1. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > drivers/crypto/s5p-sss.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c > > index ef90c58edb1f..010f1bb20dad 100644 > > --- a/drivers/crypto/s5p-sss.c > > +++ b/drivers/crypto/s5p-sss.c > > @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { > > .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | > > CRYPTO_ALG_ASYNC | > > CRYPTO_ALG_KERN_DRIVER_ONLY, > > - .cra_blocksize = AES_BLOCK_SIZE, > > + .cra_blocksize = 1, > > This makes sense but I wonder how does it work later with > s5p_aes_crypt() and its check for request length alignment > (AES_BLOCK_SIZE). With block size of 1 byte, I understand that > req->nbytes can be for example 4 bytes which is not AES block > aligned... If my reasoning is correct, then the CTR mode in s5p-sss is > not fully working. > I re-ran the kernelci.org tests with this change, and I saw no more failures. https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.3-rc1-197-gc8809c50be4f/
On Tue, 20 Aug 2019 at 12:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Tue, 20 Aug 2019 at 13:24, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > > > > Align the s5p ctr(aes) implementation with other implementations > > > of the same mode, by setting the block size to 1. > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > --- > > > drivers/crypto/s5p-sss.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c > > > index ef90c58edb1f..010f1bb20dad 100644 > > > --- a/drivers/crypto/s5p-sss.c > > > +++ b/drivers/crypto/s5p-sss.c > > > @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { > > > .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | > > > CRYPTO_ALG_ASYNC | > > > CRYPTO_ALG_KERN_DRIVER_ONLY, > > > - .cra_blocksize = AES_BLOCK_SIZE, > > > + .cra_blocksize = 1, > > > > This makes sense but I wonder how does it work later with > > s5p_aes_crypt() and its check for request length alignment > > (AES_BLOCK_SIZE). With block size of 1 byte, I understand that > > req->nbytes can be for example 4 bytes which is not AES block > > aligned... If my reasoning is correct, then the CTR mode in s5p-sss is > > not fully working. > > > > > I re-ran the kernelci.org tests with this change, and I saw no more failures. > > https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.3-rc1-197-gc8809c50be4f/ Indeed, self tests are passing. Anyway the change is correct so: Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On 20.08.2019 12:24, Krzysztof Kozlowski wrote: > On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> Align the s5p ctr(aes) implementation with other implementations >> of the same mode, by setting the block size to 1. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> drivers/crypto/s5p-sss.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c >> index ef90c58edb1f..010f1bb20dad 100644 >> --- a/drivers/crypto/s5p-sss.c >> +++ b/drivers/crypto/s5p-sss.c >> @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { >> .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | >> CRYPTO_ALG_ASYNC | >> CRYPTO_ALG_KERN_DRIVER_ONLY, >> - .cra_blocksize = AES_BLOCK_SIZE, >> + .cra_blocksize = 1, > > This makes sense but I wonder how does it work later with > s5p_aes_crypt() and its check for request length alignment > (AES_BLOCK_SIZE). With block size of 1 byte, I understand that > req->nbytes can be for example 4 bytes which is not AES block > aligned... If my reasoning is correct, then the CTR mode in s5p-sss is > not fully working. As I remember this case there are allocated buffers with len aligned up AES_BLOCK_SIZE, source data copy to one buf, hw encrypts full block, then nbytes are copy back.
On Tue, 20 Aug 2019 at 13:39, Kamil Konieczny <k.konieczny@partner.samsung.com> wrote: > > > > On 20.08.2019 12:24, Krzysztof Kozlowski wrote: > > On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > >> Align the s5p ctr(aes) implementation with other implementations > >> of the same mode, by setting the block size to 1. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> --- > >> drivers/crypto/s5p-sss.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c > >> index ef90c58edb1f..010f1bb20dad 100644 > >> --- a/drivers/crypto/s5p-sss.c > >> +++ b/drivers/crypto/s5p-sss.c > >> @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { > >> .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | > >> CRYPTO_ALG_ASYNC | > >> CRYPTO_ALG_KERN_DRIVER_ONLY, > >> - .cra_blocksize = AES_BLOCK_SIZE, > >> + .cra_blocksize = 1, > > > > This makes sense but I wonder how does it work later with > > s5p_aes_crypt() and its check for request length alignment > > (AES_BLOCK_SIZE). With block size of 1 byte, I understand that > > req->nbytes can be for example 4 bytes which is not AES block > > aligned... If my reasoning is correct, then the CTR mode in s5p-sss is > > not fully working. > > As I remember this case there are allocated buffers with len aligned up > AES_BLOCK_SIZE, source data copy to one buf, hw encrypts full block, > then nbytes are copy back. Buffer alignment is different thing and it is defined in cra_alignmask. I am talking about req->nbytes which should be aligned according to s5p_aes_crypt(). But if blocksize is 1 byte, then what possible values for req->nbytes? Best regards, Krzysztof
On 20.08.2019 13:48, Krzysztof Kozlowski wrote: > On Tue, 20 Aug 2019 at 13:39, Kamil Konieczny > <k.konieczny@partner.samsung.com> wrote: >> >> >> >> On 20.08.2019 12:24, Krzysztof Kozlowski wrote: >>> On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>> >>>> Align the s5p ctr(aes) implementation with other implementations >>>> of the same mode, by setting the block size to 1. >>>> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> --- >>>> drivers/crypto/s5p-sss.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c >>>> index ef90c58edb1f..010f1bb20dad 100644 >>>> --- a/drivers/crypto/s5p-sss.c >>>> +++ b/drivers/crypto/s5p-sss.c >>>> @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { >>>> .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | >>>> CRYPTO_ALG_ASYNC | >>>> CRYPTO_ALG_KERN_DRIVER_ONLY, >>>> - .cra_blocksize = AES_BLOCK_SIZE, >>>> + .cra_blocksize = 1, >>> >>> This makes sense but I wonder how does it work later with >>> s5p_aes_crypt() and its check for request length alignment >>> (AES_BLOCK_SIZE). With block size of 1 byte, I understand that >>> req->nbytes can be for example 4 bytes which is not AES block >>> aligned... If my reasoning is correct, then the CTR mode in s5p-sss is >>> not fully working. >> >> As I remember this case there are allocated buffers with len aligned up >> AES_BLOCK_SIZE, source data copy to one buf, hw encrypts full block, >> then nbytes are copy back. to be precise, hw encrypts align_up(req->nbytes, AES_BLOCK_SIZE) > Buffer alignment is different thing and it is defined in cra_alignmask. > I am talking about req->nbytes which should be aligned according to > s5p_aes_crypt(). But if blocksize is 1 byte, then what possible values > for req->nbytes? For AES-CTR, there are blocks of size multiply of AES_BLOCK_SIZE, with last one which can be of any size so for req->nbytes valid values are n*AES_BLOCK_SIZE + 0...AES_BLOCK_SIZE-1
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index ef90c58edb1f..010f1bb20dad 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c @@ -2173,7 +2173,7 @@ static struct crypto_alg algs[] = { .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC | CRYPTO_ALG_KERN_DRIVER_ONLY, - .cra_blocksize = AES_BLOCK_SIZE, + .cra_blocksize = 1, .cra_ctxsize = sizeof(struct s5p_aes_ctx), .cra_alignmask = 0x0f, .cra_type = &crypto_ablkcipher_type,
Align the s5p ctr(aes) implementation with other implementations of the same mode, by setting the block size to 1. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/crypto/s5p-sss.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)