diff mbox series

[2/2] crypto: s5p - use correct block size of 1 for ctr(aes)

Message ID 20190819142226.1703-3-ard.biesheuvel@linaro.org
State New
Headers show
Series drivers/crypto - s5p fixes | expand

Commit Message

Ard Biesheuvel Aug. 19, 2019, 2:22 p.m. UTC 
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(-)

-- 
2.17.1

Comments

Kamil Konieczny Aug. 20, 2019, 9:51 a.m. UTC | #1 
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>


-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
Krzysztof Kozlowski Aug. 20, 2019, 10:24 a.m. UTC | #2 
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
Ard Biesheuvel Aug. 20, 2019, 10:57 a.m. UTC | #3 
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/
Krzysztof Kozlowski Aug. 20, 2019, 11:23 a.m. UTC | #4 
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
Kamil Konieczny Aug. 20, 2019, 11:39 a.m. UTC | #5 
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.

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
Krzysztof Kozlowski Aug. 20, 2019, 11:48 a.m. UTC | #6 
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
Kamil Konieczny Aug. 20, 2019, 12:28 p.m. UTC | #7 
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

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
diff mbox series

Patch

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,