[PATCH 0/2] drivers/crypto - s5p fixes

[PATCH 0/2] drivers/crypto - s5p fixes

[Ard Biesheuvel]

Fix a couple of issues in the s5p crypto driver that were caught in fuzz
testing.

Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Vladimir Zapolskiy <vz@mleia.com>
Cc: Kamil Konieczny <k.konieczny@partner.samsung.com>
Cc: linux-samsung-soc@vger.kernel.org

Ard Biesheuvel (2):
  crypto: s5p - deal gracefully with bogus input sizes
  crypto: s5p - use correct block size of 1 for ctr(aes)

 drivers/crypto/s5p-sss.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] crypto: s5p - deal gracefully with bogus input sizes

[Ard Biesheuvel]

The s5p skcipher driver returns -EINVAL for zero length inputs, which
deviates from the behavior of the generic ECB template, and causes fuzz
tests to fail. In cases where the input is not a multiple of the AES
block size (and the chaining mode is not CTR), it prints an error to
the kernel log, which is a thing we usually try to avoid in response
to situations that can be triggered by unprivileged users.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/crypto/s5p-sss.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 9ef25230c199..ef90c58edb1f 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -2056,9 +2056,12 @@ static int s5p_aes_crypt(struct ablkcipher_request *req, unsigned long mode)
 	struct s5p_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);
 	struct s5p_aes_dev *dev = ctx->dev;
 
+	if (!req->nbytes)
+		return 0;
+
 	if (!IS_ALIGNED(req->nbytes, AES_BLOCK_SIZE) &&
 			((mode & FLAGS_AES_MODE_MASK) != FLAGS_AES_CTR)) {
-		dev_err(dev->dev, "request size is not exact amount of AES blocks\n");
+		dev_dbg(dev->dev, "request size is not exact amount of AES blocks\n");
 		return -EINVAL;
 	}
 
-- 
2.17.1

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

[Ard Biesheuvel]

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,
-- 
2.17.1

Re: [PATCH 1/2] crypto: s5p - deal gracefully with bogus input sizes

[Kamil Konieczny]

On 19.08.2019 16:22, Ard Biesheuvel wrote:
> The s5p skcipher driver returns -EINVAL for zero length inputs, which
> deviates from the behavior of the generic ECB template, and causes fuzz
> tests to fail. In cases where the input is not a multiple of the AES
> block size (and the chaining mode is not CTR), it prints an error to
> the kernel log, which is a thing we usually try to avoid in response
> to situations that can be triggered by unprivileged users.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/crypto/s5p-sss.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 9ef25230c199..ef90c58edb1f 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -2056,9 +2056,12 @@ static int s5p_aes_crypt(struct ablkcipher_request *req, unsigned long mode)
>  	struct s5p_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);
>  	struct s5p_aes_dev *dev = ctx->dev;
>  
> +	if (!req->nbytes)
> +		return 0;
> +
>  	if (!IS_ALIGNED(req->nbytes, AES_BLOCK_SIZE) &&
>  			((mode & FLAGS_AES_MODE_MASK) != FLAGS_AES_CTR)) {
> -		dev_err(dev->dev, "request size is not exact amount of AES blocks\n");
> +		dev_dbg(dev->dev, "request size is not exact amount of AES blocks\n");
>  		return -EINVAL;
>  	}

Acked-by: Kamil Konieczny <k.konieczny@partner.samsung.com>

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

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

[Kamil Konieczny]

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

Re: [PATCH 1/2] crypto: s5p - deal gracefully with bogus input sizes

[Krzysztof Kozlowski]

On Mon, 19 Aug 2019 at 16:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> The s5p skcipher driver returns -EINVAL for zero length inputs, which
> deviates from the behavior of the generic ECB template, and causes fuzz
> tests to fail. In cases where the input is not a multiple of the AES
> block size (and the chaining mode is not CTR), it prints an error to
> the kernel log, which is a thing we usually try to avoid in response
> to situations that can be triggered by unprivileged users.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/crypto/s5p-sss.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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

[Ard Biesheuvel]

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/

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

[Krzysztof Kozlowski]

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

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

[Kamil Konieczny]

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

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

[Krzysztof Kozlowski]

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

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

[Kamil Konieczny]

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

Re: [PATCH 0/2] drivers/crypto - s5p fixes

[Herbert Xu]

On Mon, Aug 19, 2019 at 05:22:24PM +0300, Ard Biesheuvel wrote:
> Fix a couple of issues in the s5p crypto driver that were caught in fuzz
> testing.
> 
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Vladimir Zapolskiy <vz@mleia.com>
> Cc: Kamil Konieczny <k.konieczny@partner.samsung.com>
> Cc: linux-samsung-soc@vger.kernel.org
> 
> Ard Biesheuvel (2):
>   crypto: s5p - deal gracefully with bogus input sizes
>   crypto: s5p - use correct block size of 1 for ctr(aes)
> 
>  drivers/crypto/s5p-sss.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt