Re: [PATCH] crypto: caam - strip input zeros from RSA input buffer

2018-04-16 Thread Martin Townsend
On Mon, Apr 16, 2018 at 10:34 AM, Horia Geantă <horia.gea...@nxp.com> wrote:
> Sometimes the provided RSA input buffer provided is not stripped
> of leading zeros. This could cause its size to be bigger than that
> of the modulus, making the HW complain:
>
> caam_jr 2142000.jr1: 4789: DECO: desc idx 7:
> Protocol Size Error - A protocol has seen an error in size. When
> running RSA, pdb size N < (size of F) when no formatting is used; or
> pdb size N < (F + 11) when formatting is used.
>
> Fix the problem by stripping off the leading zero from input data
> before feeding it to the CAAM accelerator.
>
> Fixes: 8c419778ab57e ("crypto: caam - add support for RSA algorithm")
> Cc: <sta...@vger.kernel.org> # 4.8+
> Reported-by: Martin Townsend <mtownsend1...@gmail.com>
> Link: 
> https://lkml.kernel.org/r/cabatt_ytyoryktapcb4izhnanekkgfi9xaqmjhi_n-8ywoc...@mail.gmail.com
> Signed-off-by: Horia Geantă <horia.gea...@nxp.com>
> ---
>  drivers/crypto/caam/caampkc.c | 54 
> +++
>  drivers/crypto/caam/caampkc.h |  8 +++
>  2 files changed, 62 insertions(+)
>
> diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
> index 7a897209f181..979072b25eaa 100644
> --- a/drivers/crypto/caam/caampkc.c
> +++ b/drivers/crypto/caam/caampkc.c
> @@ -166,18 +166,71 @@ static void rsa_priv_f3_done(struct device *dev, u32 
> *desc, u32 err,
> akcipher_request_complete(req, err);
>  }
>
> +static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,
> +   unsigned int nbytes,
> +   unsigned int flags)
> +{
> +   struct sg_mapping_iter miter;
> +   int lzeros, ents;
> +   unsigned int len;
> +   unsigned int tbytes = nbytes;
> +   const u8 *buff;
> +
> +   ents = sg_nents_for_len(sgl, nbytes);
> +   if (ents < 0)
> +   return ents;
> +
> +   sg_miter_start(, sgl, ents, SG_MITER_FROM_SG | flags);
> +
> +   lzeros = 0;
> +   len = 0;
> +   while (nbytes > 0) {
> +   while (len && !*buff) {
> +   lzeros++;
> +   len--;
> +   buff++;
> +   }
> +
> +   if (len && *buff)
> +   break;
> +
> +   sg_miter_next();
> +   buff = miter.addr;
> +   len = miter.length;
> +
> +   nbytes -= lzeros;
> +   lzeros = 0;
> +   }
> +
> +   miter.consumed = lzeros;
> +   sg_miter_stop();
> +   nbytes -= lzeros;
> +
> +   return tbytes - nbytes;
> +}
> +
>  static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
>  size_t desclen)
>  {
> struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
> struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
> struct device *dev = ctx->dev;
> +   struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
> struct rsa_edesc *edesc;
> gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
>GFP_KERNEL : GFP_ATOMIC;
> +   int sg_flags = (flags == GFP_ATOMIC) ? SG_MITER_ATOMIC : 0;
> int sgc;
> int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
> int src_nents, dst_nents;
> +   int lzeros;
> +
> +   lzeros = caam_rsa_count_leading_zeros(req->src, req->src_len, 
> sg_flags);
> +   if (lzeros < 0)
> +   return ERR_PTR(lzeros);
> +
> +   req->src_len -= lzeros;
> +   req->src = scatterwalk_ffwd(req_ctx->src, req->src, lzeros);
>
> src_nents = sg_nents_for_len(req->src, req->src_len);
> dst_nents = sg_nents_for_len(req->dst, req->dst_len);
> @@ -953,6 +1006,7 @@ static struct akcipher_alg caam_rsa = {
> .max_size = caam_rsa_max_size,
> .init = caam_rsa_init_tfm,
> .exit = caam_rsa_exit_tfm,
> +   .reqsize = sizeof(struct caam_rsa_req_ctx),
> .base = {
> .cra_name = "rsa",
> .cra_driver_name = "rsa-caam",
> diff --git a/drivers/crypto/caam/caampkc.h b/drivers/crypto/caam/caampkc.h
> index fd145c46eae1..82645bcf8b27 100644
> --- a/drivers/crypto/caam/caampkc.h
> +++ b/drivers/crypto/caam/caampkc.h
> @@ -95,6 +95,14 @@ struct caam_rsa_ctx {
> struct device *dev;
>  };
>
> +/**
> + * caam_rsa_req_ctx - per request context.
> + * @src: input scatterlist (stripped of leading zeros)
> + */
> +struct caam_rsa_req_ctx {
> +   struct scatterlist src[2];
> +};
> +
>  /**
>   * rsa_edesc - s/w-extended rsa descriptor
>   * @src_nents : number of segments in input scatterlist
> --
> 2.16.2
>

Using linux-imx 4.9 I can confirm that with the patch it boots to the
prompt and IMA/EVM is happily measuring files.

Tested-by: Martin Townsend <mtownsend1...@gmail.com>


Re: [PATCH v3] crypto: caam: Drop leading zero from input buffer

2018-04-15 Thread Martin Townsend
On Sun, Apr 15, 2018 at 3:12 PM, Fabio Estevam <feste...@gmail.com> wrote:
> From: Fabio Estevam <fabio.este...@nxp.com>
>
> imx6ul and imx7 report the following error:
>
> caam_jr 2142000.jr1: 4789: DECO: desc idx 7:
> Protocol Size Error - A protocol has seen an error in size. When
> running RSA, pdb size N < (size of F) when no formatting is used; or
> pdb size N < (F + 11) when formatting is used.
>
> [ cut here ]
> WARNING: CPU: 0 PID: 1 at crypto/asymmetric_keys/public_key.c:148
> public_key_verify_signature+0x27c/0x2b0
>
> This error happens because the signature contains 257 bytes, including
> a leading zero as the first element.
>
> Fix the problem by stripping off the leading zero from input data
> before feeding it to the CAAM accelerator.
>
> Fixes: 8c419778ab57e497b5 ("crypto: caam - add support for RSA algorithm")
> Cc: <sta...@vger.kernel.org>
> Reported-by: Martin Townsend <mtownsend1...@gmail.com>
> Signed-off-by: Fabio Estevam <fabio.este...@nxp.com>
> ---
> Changes since v2:
> - Check if the lenght is zero after calling caam_rsa_drop_leading_zeros()
>
>  drivers/crypto/caam/caampkc.c | 45 
> +++
>  1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
> index 7a897209..47467ff 100644
> --- a/drivers/crypto/caam/caampkc.c
> +++ b/drivers/crypto/caam/caampkc.c
> @@ -166,6 +166,14 @@ static void rsa_priv_f3_done(struct device *dev, u32 
> *desc, u32 err,
> akcipher_request_complete(req, err);
>  }
>
> +static void caam_rsa_drop_leading_zeros(const u8 **ptr, size_t *nbytes)
> +{
> +   while (!**ptr && *nbytes) {
> +   (*ptr)++;
> +   (*nbytes)--;
> +   }
> +}
> +
>  static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
>  size_t desclen)
>  {
> @@ -178,7 +186,36 @@ static struct rsa_edesc *rsa_edesc_alloc(struct 
> akcipher_request *req,
> int sgc;
> int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
> int src_nents, dst_nents;
> +   const u8 *temp;
> +   void *buffer;
> +   size_t len;
> +
> +   buffer = kzalloc(req->src_len, GFP_ATOMIC);
> +   if (!buffer)
> +   return ERR_PTR(-ENOMEM);
> +
> +   sg_copy_to_buffer(req->src, sg_nents(req->src),
> + buffer, req->src_len);
> +   temp = (u8 *)buffer;
> +   len = req->src_len;
>
> +   /*
> +* Check if the buffer contains leading zeros and if
> +* it does, drop the leading zeros
> +*/
> +   if (temp[0] == 0) {
> +   caam_rsa_drop_leading_zeros(, );
> +   if (!len) {
> +   kfree(buffer);
> +   return ERR_PTR(-ENOMEM);
> +   }
> +
> +   req->src_len = len;
> +   sg_copy_from_buffer(req->src, sg_nents(req->src),
> +   (void *)temp, req->src_len);
> +   }
> +
> +   kfree(buffer);
> src_nents = sg_nents_for_len(req->src, req->src_len);
> dst_nents = sg_nents_for_len(req->dst, req->dst_len);
>
> @@ -683,14 +720,6 @@ static void caam_rsa_free_key(struct caam_rsa_key *key)
> memset(key, 0, sizeof(*key));
>  }
>
> -static void caam_rsa_drop_leading_zeros(const u8 **ptr, size_t *nbytes)
> -{
> -   while (!**ptr && *nbytes) {
> -   (*ptr)++;
> -   (*nbytes)--;
> -   }
> -}
> -
>  /**
>   * caam_read_rsa_crt - Used for reading dP, dQ, qInv CRT members.
>   * dP, dQ and qInv could decode to less than corresponding p, q length, as 
> the
> --
> 2.7.4
>

Hi Fabio,

just tried it and it works fine on my i.MX6UL board running linux-imx
4.9 (had to manually apply the patch as it doesn't have
caam_rsa_drop_leading_zeros) so a
Reviewed and Tested-By Martin Townsend

Many Thanks,
Martin.


Re: [PATCH v2] crypto: caam: Drop leading zero from input buffer

2018-04-15 Thread Martin Townsend
On Sun, Apr 15, 2018 at 12:03 PM, Fabio Estevam <feste...@gmail.com> wrote:
> From: Fabio Estevam <fabio.este...@nxp.com>
>
> imx6ul and imx7 report the following error:
>
> caam_jr 2142000.jr1: 4789: DECO: desc idx 7:
> Protocol Size Error - A protocol has seen an error in size. When
> running RSA, pdb size N < (size of F) when no formatting is used; or
> pdb size N < (F + 11) when formatting is used.
>
> [ cut here ]
> WARNING: CPU: 0 PID: 1 at crypto/asymmetric_keys/public_key.c:148
> public_key_verify_signature+0x27c/0x2b0
>
> This error happens because the signature contains 257 bytes, including
> a leading zero as the first element.
>
> Fix the problem by stripping off the leading zero from input data
> before feeding it to the CAAM accelerator.
>
> Fixes: 8c419778ab57e497b5 ("crypto: caam - add support for RSA algorithm")
> Cc: <sta...@vger.kernel.org>
> Reported-by: Martin Townsend <mtownsend1...@gmail.com>
> Signed-off-by: Fabio Estevam <fabio.este...@nxp.com>
> ---
> Changes since v1:
> - Use a temp pointer
> - Assign len to req->src_len , so that more than one leading zero
> can be taken into account
>
>  drivers/crypto/caam/caampkc.c | 45 
> +++
>  1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
> index 7a897209..5f3e627 100644
> --- a/drivers/crypto/caam/caampkc.c
> +++ b/drivers/crypto/caam/caampkc.c
> @@ -166,6 +166,14 @@ static void rsa_priv_f3_done(struct device *dev, u32 
> *desc, u32 err,
> akcipher_request_complete(req, err);
>  }
>
> +static void caam_rsa_drop_leading_zeros(const u8 **ptr, size_t *nbytes)
> +{
> +   while (!**ptr && *nbytes) {
> +   (*ptr)++;
> +   (*nbytes)--;
> +   }
> +}
> +
>  static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
>  size_t desclen)
>  {
> @@ -178,7 +186,36 @@ static struct rsa_edesc *rsa_edesc_alloc(struct 
> akcipher_request *req,
> int sgc;
> int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
> int src_nents, dst_nents;
> +   const u8 *temp;
> +   void *buffer;
> +   size_t len;
> +
> +   buffer = kzalloc(req->src_len, GFP_ATOMIC);
> +   if (!buffer)
> +   return ERR_PTR(-ENOMEM);
> +
> +   sg_copy_to_buffer(req->src, sg_nents(req->src),
> + buffer, req->src_len);
> +   temp = (u8 *)buffer;
> +   len = req->src_len;
>
> +   /*
> +* Check if the buffer contains leading zeros and if
> +* it does, drop the leading zeros
> +*/
> +   if (temp[0] == 0) {
> +   caam_rsa_drop_leading_zeros(, );

Sorry to be a pain but looking at the other use cases for
caam_rsa_drop_leading_zeros they check len afterwards which makes more
sense to me as temp being NULL after this operation is very unlikely
:) and I suppose we are trying to catch the case where the data is all
zeroes which wouldn't be a valid signature.

> +   if (!temp) {
> +   kfree(buffer);
> +   return ERR_PTR(-ENOMEM);
> +   }
> +
> +   req->src_len = len;
> +   sg_copy_from_buffer(req->src, sg_nents(req->src),
> +   (void *)temp, req->src_len);
> +   }
> +
> +   kfree(buffer);
> src_nents = sg_nents_for_len(req->src, req->src_len);
> dst_nents = sg_nents_for_len(req->dst, req->dst_len);
>
> @@ -683,14 +720,6 @@ static void caam_rsa_free_key(struct caam_rsa_key *key)
> memset(key, 0, sizeof(*key));
>  }
>
> -static void caam_rsa_drop_leading_zeros(const u8 **ptr, size_t *nbytes)
> -{
> -   while (!**ptr && *nbytes) {
> -   (*ptr)++;
> -   (*nbytes)--;
> -   }
> -}
> -
>  /**
>   * caam_read_rsa_crt - Used for reading dP, dQ, qInv CRT members.
>   * dP, dQ and qInv could decode to less than corresponding p, q length, as 
> the
> --
> 2.7.4
>


Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-11 Thread Martin Townsend
On Wed, Apr 11, 2018 at 11:58 AM, Horia Geantă <horia.gea...@nxp.com> wrote:
> On 4/11/2018 1:36 AM, James Bottomley wrote:
>> On Tue, 2018-04-10 at 23:01 +0100, Martin Townsend wrote:
>>> Using openssl to get the signature in my x509 cert
>>>
>>>Signature Algorithm: sha256WithRSAEncryption
>>>  68:82:cc:5d:f9:ee:fb:1a:77:72:a6:a9:c6:4c:cc:d7:f6:2a:
>>>  17:a5:db:bf:5a:2b:8d:39:60:dc:a0:93:39:45:0f:bc:a7:e8:
>>>  7f:6c:06:84:2d:f3:c1:94:0a:60:56:1c:50:78:dc:34:d1:87:
>>>
>>> So there's an extra 0x00 and the signature is 257 bytes so I guess
>>> this is upsetting CAAM, just need to work out where it's coming from,
>>> or whether it's valid and CAAM should be handling it.
>>
>> A signature is just a bignum so leading zeros are permitted because
>> it's the same numeric value; however, there are normalization
>> procedures that require stripping the leading zeros, say before doing a
>> hash or other operation which would be affected by them.
>>
>> CAAM should definitely handle it on the "be liberal in what you accept"
>>  principle.  The kernel should probably remove the leading zeros on the
>> "be conservative in what you do" part of the same principle.
>>
> Looking at the generic SW implementation (crypto/rsa.c, rsa_verify()), leading
> zeros are removed:
> s = mpi_read_raw_from_sgl(req->src, req->src_len);
>
> CAAM implementation of rsa is not doing this (though it is removing leading
> zeros when reading public, private keys).
> It has to be fixed. Thanks for the report.
>

Do you have any idea when a fix will be available? I'm happy to test
on my setup here.

>>>   I notice that in my stack trace I have pkcs1pad_verify which
>>> suggests some sort of padding?
>>
>> Yes, RSA has various forms of padding because the information being
>> encrypted is usually much smaller than the encryption unit; PKCS1 is
>> the most common (although its now deprecated in favour of OAEP because
>> of all the padding oracle problems).
>>
> RSA padding has been intentionally added as a template, wrapping "textbook"
> (raw) RSA primitives.
> For PKCS#1 v1.5, a template instantiation is called pkcs1pad(rsa, hash_alg).
>
> Currently in kernel the only supported RSA padding scheme is PKCS#1 v1.5.
> When implemented, another scheme - for e.g. OAEP - would be added in a similar
> way, as a template: oaep(rsa, ...).
>
> Horia


Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-11 Thread Martin Townsend
Hi James,

On Tue, Apr 10, 2018 at 11:36 PM, James Bottomley
<james.bottom...@hansenpartnership.com> wrote:
> On Tue, 2018-04-10 at 23:01 +0100, Martin Townsend wrote:
>> Using openssl to get the signature in my x509 cert
>>
>>Signature Algorithm: sha256WithRSAEncryption
>>  68:82:cc:5d:f9:ee:fb:1a:77:72:a6:a9:c6:4c:cc:d7:f6:2a:
>>  17:a5:db:bf:5a:2b:8d:39:60:dc:a0:93:39:45:0f:bc:a7:e8:
>>  7f:6c:06:84:2d:f3:c1:94:0a:60:56:1c:50:78:dc:34:d1:87:
>>
>> So there's an extra 0x00 and the signature is 257 bytes so I guess
>> this is upsetting CAAM, just need to work out where it's coming from,
>> or whether it's valid and CAAM should be handling it.
>
> A signature is just a bignum so leading zeros are permitted because
> it's the same numeric value; however, there are normalization
> procedures that require stripping the leading zeros, say before doing a
> hash or other operation which would be affected by them.
>
> CAAM should definitely handle it on the "be liberal in what you accept"
>  principle.  The kernel should probably remove the leading zeros on the
> "be conservative in what you do" part of the same principle.
>
>>   I notice that in my stack trace I have pkcs1pad_verify which
>> suggests some sort of padding?
>
> Yes, RSA has various forms of padding because the information being
> encrypted is usually much smaller than the encryption unit; PKCS1 is
> the most common (although its now deprecated in favour of OAEP because
> of all the padding oracle problems).
>
> James
>

Thanks for the info, so the leading zero needs to be removed.


Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-10 Thread Martin Townsend
On Tue, Apr 10, 2018 at 7:43 PM, Martin Townsend
<mtownsend1...@gmail.com> wrote:
> Hi Fabio,
>
> On Tue, Apr 10, 2018 at 7:22 PM, Fabio Estevam <feste...@gmail.com> wrote:
>> Hi Martin,
>>
>> On Tue, Apr 10, 2018 at 2:06 PM, Martin Townsend
>> <mtownsend1...@gmail.com> wrote:
>>> Hi Fabio,
>>>
>>> On Tue, Apr 10, 2018 at 5:59 PM, Fabio Estevam <feste...@gmail.com> wrote:
>>>> Hi Martin,
>>>>
>>>> On Mon, Apr 9, 2018 at 5:41 AM, Martin Townsend <mtownsend1...@gmail.com> 
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>> I'm trying to get to the bottom of an issue I'm seeing when enabling
>>>>> the CAAM in the kernel with IMA/EVM enabled.  I'm using the official
>>>>> NXP (imx_4.9.11_1.0.0_ga) vendor Kernel.
>>>>
>>>> Does it work better if you try mainline kernel instead?
>>>
>>> I had a few issues getting mainline working, the board kept resetting,
>>
>> Let's try to fix this reset problem then :-)
>
> My preference would be mainline, no offence to the NXP kernel but it
> would be good to use the LTSI kernel so we get security updates etc :)
>  The reset was something to do with USB but that was as far as I got.
>
>>
>>> when I checked there are lots of patches in the NXP kernel not in
>>> mainline.   This CAAM problem does occur really early in the boot so
>>> just for an experiment its worth a try.
>>
>> Ok, I just applied this patch that adds CAAM for mx6ull against linux-next:
>>
>> http://code.bulix.org/rjkzt5-317022
>>
>> and I see the following issue with cfg80211 certificate, but I do not
>> get a reset as you reported:
>
> The reset (which is not the reset described above) occurs because I
> have IMA enabled and because it can't load the x509 certificate it
> can't verify init on the filesystem and hence it panics and resets.
>
> The message you are seeing below is the same as I'm seeing.  I'm not
> sure if you've seen my later posts but I put some debug statements and
> could see that in my case the signature is 257 bytes and key 270 bytes
> which is at odds with the error message.  Reading a post some
> signatures can contain extra information beside the signature so I'm
> wondering if the 257 bytes is a 256 byte signature plus a byte which
> indicates the encryption used to create the signature or something
> like that.
>

A hexdump of the signature reveals a 0x00 at the start

int public_key_verify_signature(const struct public_key *pkey,
const struct public_key_signature *sig)
{
...
print_hex_dump(KERN_ERR, "signature", DUMP_PREFIX_OFFSET, 16, 1,
   sig->s, sig->s_size, true);
...
=>

signature: 00 68 82 cc 5d f9 ee fb 1a 77 72 a6 a9 c6 4c cc
.h..]wr...L.
signature0010: d7 f6 2a 17 a5 db bf 5a 2b 8d 39 60 dc a0 93 39
..*Z+.9`...9
signature0020: 45 0f bc a7 e8 7f 6c 06 84 2d f3 c1 94 0a 60 56
E.l..-`V.
...


Using openssl to get the signature in my x509 cert

   Signature Algorithm: sha256WithRSAEncryption
 68:82:cc:5d:f9:ee:fb:1a:77:72:a6:a9:c6:4c:cc:d7:f6:2a:
 17:a5:db:bf:5a:2b:8d:39:60:dc:a0:93:39:45:0f:bc:a7:e8:
 7f:6c:06:84:2d:f3:c1:94:0a:60:56:1c:50:78:dc:34:d1:87:

So there's an extra 0x00 and the signature is 257 bytes so I guess
this is upsetting CAAM, just need to work out where it's coming from,
or whether it's valid and CAAM should be handling it.  I notice that
in my stack trace I have pkcs1pad_verify which suggests some sort of
padding?

>>
>> [2.999416] caam_jr 2142000.jr1: 4789: DECO: desc idx 7:
>> Protocol Size Error - A protocol has seen an error in size. When
>> running RSA, pdb size N < (size of F) when no formatting is used; or
>> pdb si
>> ze N < (F + 11) when formatting is used.
>> [3.022168] [ cut here ]
>> [3.027247] WARNING: CPU: 0 PID: 1 at
>> crypto/asymmetric_keys/public_key.c:148
>> public_key_verify_signature+0x27c/0x2b0
>> [3.038075] Modules linked in:
>> [3.041226] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 4.16.0-next-20180410-2-gf0ccf31-dirty #223
>> [3.050413] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
>> [3.056643] Backtrace:
>> [3.059173] [] (dump_backtrace) from []
>> (show_stack+0x18/0x1c)
>> [3.066802]  r7: r6:6153 r5: r4:c107ae78
>> [3.072523] [] (show_stack) from []
>> (dump_stack+0xb4/0xe8)
>> [3.079810] [] (dump_stack) from [] 
>> (__warn+0x104/0x130)
>> [3.086922]  r9:d604dc94 r8:0094 r7:0009 r6:c0d3aea8
>> r

Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-10 Thread Martin Townsend
Hi Fabio,

On Tue, Apr 10, 2018 at 7:22 PM, Fabio Estevam <feste...@gmail.com> wrote:
> Hi Martin,
>
> On Tue, Apr 10, 2018 at 2:06 PM, Martin Townsend
> <mtownsend1...@gmail.com> wrote:
>> Hi Fabio,
>>
>> On Tue, Apr 10, 2018 at 5:59 PM, Fabio Estevam <feste...@gmail.com> wrote:
>>> Hi Martin,
>>>
>>> On Mon, Apr 9, 2018 at 5:41 AM, Martin Townsend <mtownsend1...@gmail.com> 
>>> wrote:
>>>> Hi,
>>>>
>>>> I'm trying to get to the bottom of an issue I'm seeing when enabling
>>>> the CAAM in the kernel with IMA/EVM enabled.  I'm using the official
>>>> NXP (imx_4.9.11_1.0.0_ga) vendor Kernel.
>>>
>>> Does it work better if you try mainline kernel instead?
>>
>> I had a few issues getting mainline working, the board kept resetting,
>
> Let's try to fix this reset problem then :-)

My preference would be mainline, no offence to the NXP kernel but it
would be good to use the LTSI kernel so we get security updates etc :)
 The reset was something to do with USB but that was as far as I got.

>
>> when I checked there are lots of patches in the NXP kernel not in
>> mainline.   This CAAM problem does occur really early in the boot so
>> just for an experiment its worth a try.
>
> Ok, I just applied this patch that adds CAAM for mx6ull against linux-next:
>
> http://code.bulix.org/rjkzt5-317022
>
> and I see the following issue with cfg80211 certificate, but I do not
> get a reset as you reported:

The reset (which is not the reset described above) occurs because I
have IMA enabled and because it can't load the x509 certificate it
can't verify init on the filesystem and hence it panics and resets.

The message you are seeing below is the same as I'm seeing.  I'm not
sure if you've seen my later posts but I put some debug statements and
could see that in my case the signature is 257 bytes and key 270 bytes
which is at odds with the error message.  Reading a post some
signatures can contain extra information beside the signature so I'm
wondering if the 257 bytes is a 256 byte signature plus a byte which
indicates the encryption used to create the signature or something
like that.

>
> [2.999416] caam_jr 2142000.jr1: 4789: DECO: desc idx 7:
> Protocol Size Error - A protocol has seen an error in size. When
> running RSA, pdb size N < (size of F) when no formatting is used; or
> pdb si
> ze N < (F + 11) when formatting is used.
> [3.022168] [ cut here ]
> [3.027247] WARNING: CPU: 0 PID: 1 at
> crypto/asymmetric_keys/public_key.c:148
> public_key_verify_signature+0x27c/0x2b0
> [3.038075] Modules linked in:
> [3.041226] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 4.16.0-next-20180410-2-gf0ccf31-dirty #223
> [3.050413] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [3.056643] Backtrace:
> [3.059173] [] (dump_backtrace) from []
> (show_stack+0x18/0x1c)
> [3.066802]  r7: r6:6153 r5: r4:c107ae78
> [3.072523] [] (show_stack) from []
> (dump_stack+0xb4/0xe8)
> [3.079810] [] (dump_stack) from [] 
> (__warn+0x104/0x130)
> [3.086922]  r9:d604dc94 r8:0094 r7:0009 r6:c0d3aea8
> r5: r4:
> [3.094728] [] (__warn) from []
> (warn_slowpath_null+0x44/0x50)
> [3.102356]  r8:c1008908 r7:d67846c0 r6:c040bbc4 r5:0094 r4:c0d3aea8
> [3.109120] [] (warn_slowpath_null) from []
> (public_key_verify_signature+0x27c/0x2b0)
> [3.118745]  r6:4789 r5:d6782f00 r4:d6787f40
> [3.123428] [] (public_key_verify_signature) from
> [] (x509_check_for_self_signed+0xc8/0x104)
> [3.133664]  r10:d602f000 r9:c0bcb1d0 r8:02a8 r7:d6787f00
> r6:d6787f40 r5:
> [3.141543]  r4:d6782d80
> [3.144140] [] (x509_check_for_self_signed) from
> [] (x509_cert_parse+0x11c/0x190)
> [3.153415]  r7:c0bcb1d0 r6:d6787f80 r5:d6782d80 r4:d6787f00
> [3.159138] [] (x509_cert_parse) from []
> (x509_key_preparse+0x1c/0x194)
> [3.167550]  r9:c0bcb1d0 r8:c10235dc r7:d604de30 r6:c1026a84
> r5:d604de30 r4:c1026af0
> [3.175357] [] (x509_key_preparse) from []
> (asymmetric_key_preparse+0x50/0x80)
> [3.184376]  r9:c0bcb1d0 r8:c10235dc r7:d604de30 r6:c1026a84
> r5:c1008908 r4:c1026af0
> [3.192187] [] (asymmetric_key_preparse) from
> [] (key_create_or_update+0x138/0x404)
> [3.201638]  r7:d6495601 r6:d6495600 r5:c1008908 r4:c1026a8c
> [3.207366] [] (key_create_or_update) from []
> (regulatory_init_db+0xf4/0x1e8)
> [3.216303]  r10:000e r9:1f03 r8:c0d1d144 r7:c17f1e7c
> r6:c0bcb478 r5:02a8
> [3.224180]  r4:c0bcb1d0
> [3.226780] [] (regulatory_init_db) from []
> (

Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-10 Thread Martin Townsend
Hi Fabio,

On Tue, Apr 10, 2018 at 5:59 PM, Fabio Estevam <feste...@gmail.com> wrote:
> Hi Martin,
>
> On Mon, Apr 9, 2018 at 5:41 AM, Martin Townsend <mtownsend1...@gmail.com> 
> wrote:
>> Hi,
>>
>> I'm trying to get to the bottom of an issue I'm seeing when enabling
>> the CAAM in the kernel with IMA/EVM enabled.  I'm using the official
>> NXP (imx_4.9.11_1.0.0_ga) vendor Kernel.
>
> Does it work better if you try mainline kernel instead?

I had a few issues getting mainline working, the board kept resetting,
when I checked there are lots of patches in the NXP kernel not in
mainline.   This CAAM problem does occur really early in the boot so
just for an experiment its worth a try.


Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-09 Thread Martin Townsend
Hi Mike,

On Mon, Apr 9, 2018 at 5:53 PM, Mike Rapoport <r...@linux.vnet.ibm.com> wrote:
> (added CAAM maintainers)
>
> On Mon, Apr 09, 2018 at 03:10:11PM +0100, Martin Townsend wrote:
>> Hi Mimi,
>>
>> On Mon, Apr 9, 2018 at 1:46 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
>> > On Mon, 2018-04-09 at 09:41 +0100, Martin Townsend wrote:
>> >> Hi,
>> >>
>> >> I'm trying to get to the bottom of an issue I'm seeing when enabling
>> >> the CAAM in the kernel with IMA/EVM enabled.  I'm using the official
>> >> NXP (imx_4.9.11_1.0.0_ga) vendor Kernel.
>> >>
>
> [ snip ]
>
>> I think the integrity error message is a side effect of the previous
>> error, ie we are getting this error message because the CAAM is
>> failing to verify the certificates signature and hence IMA fails to
>> load the certificate onto the keyring.  If I disable CAAM then
>> everything works as expected.  What I'm trying to get to the bottom of
>> is why CAAM is failing to verify the signature.  Further below in the
>> email I have determined that the signature is 257 bytes do you think
>> this is correct?  I've read a post here:
>> https://crypto.stackexchange.com/questions/3505/what-is-the-length-of-an-rsa-signature
>>
>> That says that for PKCS#1 the signature should always be of the size
>> of the modulus, then it goes on to say: "In some protocols, there can
>> be some wrapping around the signature, e.g. to indicate which
>> algorithm was used,"  I'm wondering if that's what I'm seeing, an
>> extra byte in the signature that is the type of algorithm used but
>> this extra byte is also passed to the CAAM and causes it to fail as
>> then the signature is now larger than the modulus.  But I don't know
>> what I can do about this, I'm not even sure what protocol is being
>> used to generate this extra byte, any suggestions on how to find this
>> out would be appreciated.
>
> I don't really understand crypto and I maybe talking complete nonsense, but
> judging by diffstat between the imx_4.9.11_1.0.0_ga and mainline, the CAAM
> driver had a lot of additions since then :)
>
> The caam_rsa_dec function gained form 2 and form of 3 RSA decoding.
> That might explain your issue...
>

Thanks very interesting, definitely worth investigating, hopefully
they will backport fairly easily.

I'm not seeing any caam_rsa_dec but I do see caam_rsa_enc, is this
expected when verifying signatures?
public_key_verify_signature -> pkcs1pad_verify -> caam_rsa_enc

>> >
>> >
>> >> I put a dump_stack in the error handling routine in CAAM and in the
>> >> caam_jr_enqueue to get (I removed some of the caam_jr_enqueue
>> >> dump_stacks during initialisation to highlight the one of interest)
>
> Does any of the other caam_jr_enqueue stacks include
> load_system_certificate_list() call?

Not that I've seen, the only stack trace I've seen stem from
integrity_load_x509; the IMA x509 certificate is the first thing that
gets processed by CAAM.

> If you have a x509 certificate built into the kernel I presume it will also
> pass through caam_rsa_dec.
>
> You can also try using the built-in certificate as IMA x509 certificate and 
> see
> if it changes anything.

I'm compiling the certificate into the kernel as it's required so
early in the boot for IMA.


>
>> >> caam 214.caam: ERA source: CCBVID.
>> >> caam 214.caam: Entropy delay = 3200
>> >> caam 214.caam: Instantiated RNG4 SH0
>> >> caam 214.caam: Instantiated RNG4 SH1
>> >> caam 214.caam: device ID = 0x0a160300 (Era 8)
>> >> caam 214.caam: job rings = 3, qi = 0
>> >> caam algorithms registered in /proc/crypto
>> >> caam_jr_enqueue
>> >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> >> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> >> caam_jr_enqueue
>> >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> >> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> >> caam_jr 2141000.jr0: registering rng-caam
>> >> caam 214.caam: caam pkc algorithms registered in /proc/crypto
>> >> caam_jr_enqueue
>> >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> >> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> >> caam_rsa_set_pub_key
>> >> caam_rsa_max_size
>> >> caam_rsa_enc
>> >> caam_jr_enqueue
>> >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>&g

Re: CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-09 Thread Martin Townsend
Hi Mimi,

On Mon, Apr 9, 2018 at 1:46 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> On Mon, 2018-04-09 at 09:41 +0100, Martin Townsend wrote:
>> Hi,
>>
>> I'm trying to get to the bottom of an issue I'm seeing when enabling
>> the CAAM in the kernel with IMA/EVM enabled.  I'm using the official
>> NXP (imx_4.9.11_1.0.0_ga) vendor Kernel.
>>
>> Here's the error message I'm getting.
>>
>> caam_jr 2142000.jr1: 4789: DECO: desc idx 7: Protocol Size Error -
>> A protocol has seen an error in size. When running RSA, pdb size N <
>> (size of F) when no formatting is used; or pdb size N < (F + 11) when
>> formatting is used.
>> integrity: Problem loading X.509 certificate (-129): /etc/keys/ima-x509.der
>
> This error message indicates that the kernel is trying to load a key
> onto the IMA keyring, but any key added to the trusted IMA keyring
> (.ima) must be signed by a key on the builtin (or secondary) keyring.
>
> Please verify that the public key used to sign the ima-x509.der is on
> the builtin keyring (eg. "keyctl show %keyring:.builtin_trusted_keys)
> or the secondary keyring.  Depending on how the kernel was configured,
> loading the public CA onto the builtin (or secondary) keyring might be
> possible.
>
> Some distros are carrying patches which load the UEFI keys onto the
> secondary keyring.  The other (preferred) method would be to insert
> the CA certificate into the kernel and resign the kernel.  This
> requires reserving memory for the key when the kernel is built.
>
> CONFIG_SYSTEM_EXTRA_CERTIFICATE=y
> CONFIG_SYSTEM_EXTRA_CERTIFICATE_SIZE=4096
>
> Mimi

I think the integrity error message is a side effect of the previous
error, ie we are getting this error message because the CAAM is
failing to verify the certificates signature and hence IMA fails to
load the certificate onto the keyring.  If I disable CAAM then
everything works as expected.  What I'm trying to get to the bottom of
is why CAAM is failing to verify the signature.  Further below in the
email I have determined that the signature is 257 bytes do you think
this is correct?  I've read a post here:
https://crypto.stackexchange.com/questions/3505/what-is-the-length-of-an-rsa-signature

That says that for PKCS#1 the signature should always be of the size
of the modulus, then it goes on to say: "In some protocols, there can
be some wrapping around the signature, e.g. to indicate which
algorithm was used,"  I'm wondering if that's what I'm seeing, an
extra byte in the signature that is the type of algorithm used but
this extra byte is also passed to the CAAM and causes it to fail as
then the signature is now larger than the modulus.  But I don't know
what I can do about this, I'm not even sure what protocol is being
used to generate this extra byte, any suggestions on how to find this
out would be appreciated.

>
>
>> I put a dump_stack in the error handling routine in CAAM and in the
>> caam_jr_enqueue to get (I removed some of the caam_jr_enqueue
>> dump_stacks during initialisation to highlight the one of interest)
>>
>> caam 214.caam: ERA source: CCBVID.
>> caam 214.caam: Entropy delay = 3200
>> caam 214.caam: Instantiated RNG4 SH0
>> caam 214.caam: Instantiated RNG4 SH1
>> caam 214.caam: device ID = 0x0a160300 (Era 8)
>> caam 214.caam: job rings = 3, qi = 0
>> caam algorithms registered in /proc/crypto
>> caam_jr_enqueue
>> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> caam_jr_enqueue
>> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> caam_jr 2141000.jr0: registering rng-caam
>> caam 214.caam: caam pkc algorithms registered in /proc/crypto
>> caam_jr_enqueue
>> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> caam_rsa_set_pub_key
>> caam_rsa_max_size
>> caam_rsa_enc
>> caam_jr_enqueue
>> CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
>> Hardware name: Freescale i.MX6 UltraLite (Device Tree)
>> [<8010dd8c>] (unwind_backtrace) from [<8010b8cc>] (show_stack+0x10/0x14)
>> [<8010b8cc>] (show_stack) from [<80608f74>] (caam_jr_enqueue+0x3c/0x2bc)
>> [<80608f74>] (caam_jr_enqueue) from [<8061fb84>] (caam_rsa_enc+0x210/0x3ac)
>> [<8061fb84>] (caam_rsa_enc) from [<803ed910>] (pkcs1pad_verify+0xa8/0xe4)
>> [<803ed910>] (pkcs1pad_verify) from [<804134e4>]
>> (public_key_verify_signature+0x17c/0x248)
>> [<804134e4>

CAAM and IMA/EVM : caam_rsa_enc: DECO: desc idx 7: Protocol Size Error

2018-04-09 Thread Martin Townsend
Hi,

I'm trying to get to the bottom of an issue I'm seeing when enabling
the CAAM in the kernel with IMA/EVM enabled.  I'm using the official
NXP (imx_4.9.11_1.0.0_ga) vendor Kernel.

Here's the error message I'm getting.

caam_jr 2142000.jr1: 4789: DECO: desc idx 7: Protocol Size Error -
A protocol has seen an error in size. When running RSA, pdb size N <
(size of F) when no formatting is used; or pdb size N < (F + 11) when
formatting is used.
integrity: Problem loading X.509 certificate (-129): /etc/keys/ima-x509.der


I put a dump_stack in the error handling routine in CAAM and in the
caam_jr_enqueue to get (I removed some of the caam_jr_enqueue
dump_stacks during initialisation to highlight the one of interest)

caam 214.caam: ERA source: CCBVID.
caam 214.caam: Entropy delay = 3200
caam 214.caam: Instantiated RNG4 SH0
caam 214.caam: Instantiated RNG4 SH1
caam 214.caam: device ID = 0x0a160300 (Era 8)
caam 214.caam: job rings = 3, qi = 0
caam algorithms registered in /proc/crypto
caam_jr_enqueue
CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
Hardware name: Freescale i.MX6 UltraLite (Device Tree)
caam_jr_enqueue
CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
Hardware name: Freescale i.MX6 UltraLite (Device Tree)
caam_jr 2141000.jr0: registering rng-caam
caam 214.caam: caam pkc algorithms registered in /proc/crypto
caam_jr_enqueue
CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
Hardware name: Freescale i.MX6 UltraLite (Device Tree)
caam_rsa_set_pub_key
caam_rsa_max_size
caam_rsa_enc
caam_jr_enqueue
CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.11-1.0.0+gc27010d #4
Hardware name: Freescale i.MX6 UltraLite (Device Tree)
[<8010dd8c>] (unwind_backtrace) from [<8010b8cc>] (show_stack+0x10/0x14)
[<8010b8cc>] (show_stack) from [<80608f74>] (caam_jr_enqueue+0x3c/0x2bc)
[<80608f74>] (caam_jr_enqueue) from [<8061fb84>] (caam_rsa_enc+0x210/0x3ac)
[<8061fb84>] (caam_rsa_enc) from [<803ed910>] (pkcs1pad_verify+0xa8/0xe4)
[<803ed910>] (pkcs1pad_verify) from [<804134e4>]
(public_key_verify_signature+0x17c/0x248)
[<804134e4>] (public_key_verify_signature) from [<804132a0>]
(restrict_link_by_signature+0xa8/0xd4)
[<804132a0>] (restrict_link_by_signature) from [<803cadd4>]
(key_create_or_update+0x12c/0x370)
[<803cadd4>] (key_create_or_update) from [<80c253a0>]
(integrity_load_x509+0x70/0xc4)
[<80c253a0>] (integrity_load_x509) from [<80c256bc>] (ima_load_x509+0x28/0x3c)
[<80c256bc>] (ima_load_x509) from [<80c2510c>] (integrity_load_keys+0x8/0x10)
[<80c2510c>] (integrity_load_keys) from [<80c00de0>]
(kernel_init_freeable+0x1bc/0x1cc)
[<80c00de0>] (kernel_init_freeable) from [<80844ccc>] (kernel_init+0x8/0x114)
[<80844ccc>] (kernel_init) from [<801078d8>] (ret_from_fork+0x14/0x3c)
CPU: 0 PID: 112 Comm: irq/214-2142000 Not tainted 4.9.11-1.0.0+gc27010d #4
Hardware name: Freescale i.MX6 UltraLite (Device Tree)
[<8010dd8c>] (unwind_backtrace) from [<8010b8cc>] (show_stack+0x10/0x14)
[<8010b8cc>] (show_stack) from [<8060a0e8>] (report_deco_status+0x30/0xf8)
[<8060a0e8>] (report_deco_status) from [<8061f3c8>] (rsa_pub_done+0x20/0x60)
[<8061f3c8>] (rsa_pub_done) from [<80608d94>] (caam_jr_threadirq+0x1dc/0x29c)
[<80608d94>] (caam_jr_threadirq) from [<80165dcc>] (irq_thread_fn+0x1c/0x54)
[<80165dcc>] (irq_thread_fn) from [<80166024>] (irq_thread+0x114/0x1d4)
[<80166024>] (irq_thread) from [<801415b4>] (kthread+0xe4/0xec)
[<801415b4>] (kthread) from [<801078d8>] (ret_from_fork+0x14/0x3c)
caam_jr 2142000.jr1: 4789: DECO: desc idx 7: Protocol Size Error -
A protocol has seen an error in size. When running RSA, pdb size N <
(size of F) when no formatting is used; or pdb size N < (F + 11) when
formatting is used.
integrity: Problem loading X.509 certificate (-129): /etc/keys/ima-x509.der
[<8010dd8c>] (unwind_backtrace) from [<8010b8cc>] (show_stack+0x10/0x14)
[<8010b8cc>] (show_stack) from [<8060a0d0>] (report_deco_status+0x30/0xf8)
[<8060a0d0>] (report_deco_status) from [<8061db94>] (rsa_pub_done+0x20/0x60)
[<8061db94>] (rsa_pub_done) from [<80608d94>] (caam_jr_threadirq+0x1dc/0x29c)
[<80608d94>] (caam_jr_threadirq) from [<80165dcc>] (irq_thread_fn+0x1c/0x54)
[<80165dcc>] (irq_thread_fn) from [<80166024>] (irq_thread+0x114/0x1d4)
[<80166024>] (irq_thread) from [<801415b4>] (kthread+0xe4/0xec)
[<801415b4>] (kthread) from [<801078d8>] (ret_from_fork+0x14/0x3c)

So it's failing to verify the signature on my x509 certificate because
pdb size M < (size of F) ...

On the NXP mailing lists there is someone with a similar issue and the
response was
“The error appears to be saying that the public RSA modulus is shorter
than the signature being verified.

RSA uses modular arithmetic, and all valid values are smaller than the
public RSA modulus. However, the signature (F in the error message
below), is too large.

The customer should try to determine why their public RSA Modulus (N)
is shorter than the signature (F).”

I have to admit I'm not really sure