by Gary R Hook

[permalink] [raw]

Subject: Re: [PATCH 3/6] crypto: ccp - Add support for RSA on the CCP

On 10/13/2016 01:25 PM, Stephan Mueller wrote:
> Am Donnerstag, 13. Oktober 2016, 09:53:09 CEST schrieb Gary R Hook:
>
> Hi Gary,
>
>> Wire up the v3 CCP as a cipher provider.
>>
>> Signed-off-by: Gary R Hook <[email protected]>
>> ---
>>
>> ...snip...
>>
>> +}
>> +
>> +static void ccp_free_mpi_key(struct ccp_rsa_key *key)
>> +{
>> + mpi_free(key->d);
>> + key->d = NULL;
>> + mpi_free(key->e);
>> + key->e = NULL;
>> + mpi_free(key->n);
>> + key->n = NULL;
>> +}
>
> Could you please see whether that function can be turned into a common
> function call? crypto/rsa.c implements the same code in rsa_free_mpi_key.

I am happy to do so, but was unsure of protocol. rsa.c is in a module, which
makes my module depend upon another. I do not want to do that. And moving
this function elsewhere makes no sense.

I would go with an inline function, but there's no obvious place for it.
The RSA software implementation uses the MPI library, but there's no
requirement to do so (witness the qat driver). Thus, an inline function
can't
be put in internal/rsa.h without moving the rsa_mpi_key definition and
referencing mpi.h.

I think that RSA+MPI things, such as rsa_mpi_key and this function, could go
into internal/rsa.h, but it would be necessary to #include mpi.h.

Or: create a new include file that contains these (and any other) RSA/MPI
amalgams.

Which would you prefer?

>> +
>> +static int ccp_check_key_length(unsigned int len)
>> +{
>> + /* In bits */
>> + if (len < 8 || len > 16384)
>> + return -EINVAL;
>> + return 0;
>> +}
>> +
>> +static void ccp_rsa_free_key_bufs(struct ccp_ctx *ctx)
>> +{
>> + /* Clean up old key data */
>> + kfree(ctx->u.rsa.e_buf);
>> + ctx->u.rsa.e_buf = NULL;
>> + ctx->u.rsa.e_len = 0;
>> + kfree(ctx->u.rsa.n_buf);
>> + ctx->u.rsa.n_buf = NULL;
>> + ctx->u.rsa.n_len = 0;
>> + kfree(ctx->u.rsa.d_buf);
>
> kzfree, please.

Of course. Done.

>>
>> ...snip...
>>
>> +}
>> +
>> +static struct akcipher_alg rsa = {
>> + .encrypt = ccp_rsa_encrypt,
>> + .decrypt = ccp_rsa_decrypt,
>> + .sign = NULL,
>> + .verify = NULL,
>> + .set_pub_key = ccp_rsa_setpubkey,
>> + .set_priv_key = ccp_rsa_setprivkey,
>> + .max_size = ccp_rsa_maxsize,
>> + .init = ccp_rsa_init_tfm,
>> + .exit = ccp_rsa_exit_tfm,
>> + .reqsize = sizeof(struct ccp_rsa_req_ctx),
>> + .base = {
>> + .cra_name = "rsa",
>> + .cra_driver_name = "rsa-ccp",
>> + .cra_priority = 100,
>
> Are you sure you want to leave it at 100? With this value, it will content
> with the C implementation.

No, I don't. Our other functions are at 300 (CCP_CRA_PRIORITY), which is
what
this should be.

>
>> + .cra_module = THIS_MODULE,
>> + .cra_ctxsize = sizeof(struct ccp_ctx),
>> + },
>> +};
>> +
>> ...snip...
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> Ciao
> Stephan
>

Thank you. I hope snipping is acceptable...

--
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer

2016-10-14 03:35:14

by Tom Lendacky

[permalink] [raw]

Subject: Re: [PATCH 2/6] crypto: ccp - Remove unneeded sign-extension support

On 10/13/2016 09:53 AM, Gary R Hook wrote:
> The reverse-get/set functions can be simplified by
> eliminating unused code.
>
>
> Signed-off-by: Gary R Hook <[email protected]>
> ---
> drivers/crypto/ccp/ccp-ops.c | 145 +++++++++++++++++-------------------------
> 1 file changed, 59 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
> index 8fedb14..82cc637 100644
> --- a/drivers/crypto/ccp/ccp-ops.c
> +++ b/drivers/crypto/ccp/ccp-ops.c
> @@ -198,62 +198,46 @@ static void ccp_get_dm_area(struct ccp_dm_workarea *wa, unsigned int wa_offset,
> }
>
> static int ccp_reverse_set_dm_area(struct ccp_dm_workarea *wa,
> + unsigned int wa_offset,
> struct scatterlist *sg,
> - unsigned int len, unsigned int se_len,
> - bool sign_extend)
> + unsigned int sg_offset,
> + unsigned int len)
> {
> - unsigned int nbytes, sg_offset, dm_offset, sb_len, i;
> - u8 buffer[CCP_REVERSE_BUF_SIZE];
> -
> - if (WARN_ON(se_len > sizeof(buffer)))
> - return -EINVAL;
> -
> - sg_offset = len;
> - dm_offset = 0;
> - nbytes = len;
> - while (nbytes) {
> - sb_len = min_t(unsigned int, nbytes, se_len);
> - sg_offset -= sb_len;
> -
> - scatterwalk_map_and_copy(buffer, sg, sg_offset, sb_len, 0);
> - for (i = 0; i < sb_len; i++)
> - wa->address[dm_offset + i] = buffer[sb_len - i - 1];
> -
> - dm_offset += sb_len;
> - nbytes -= sb_len;
> -
> - if ((sb_len != se_len) && sign_extend) {
> - /* Must sign-extend to nearest sign-extend length */
> - if (wa->address[dm_offset - 1] & 0x80)
> - memset(wa->address + dm_offset, 0xff,
> - se_len - sb_len);
> - }
> + u8 *p, *q;
> +
> + ccp_set_dm_area(wa, wa_offset, sg, sg_offset, len);
> +
> + p = wa->address + wa_offset;
> + q = p + len - 1;
> + while (p < q) {
> + *p = *p ^ *q;
> + *q = *p ^ *q;
> + *p = *p ^ *q;
> + p++;
> + q--;
> }
> -
> return 0;
> }
>
> static void ccp_reverse_get_dm_area(struct ccp_dm_workarea *wa,
> + unsigned int wa_offset,
> struct scatterlist *sg,
> + unsigned int sg_offset,
> unsigned int len)
> {
> - unsigned int nbytes, sg_offset, dm_offset, sb_len, i;
> - u8 buffer[CCP_REVERSE_BUF_SIZE];
> -
> - sg_offset = 0;
> - dm_offset = len;
> - nbytes = len;
> - while (nbytes) {
> - sb_len = min_t(unsigned int, nbytes, sizeof(buffer));
> - dm_offset -= sb_len;
> -
> - for (i = 0; i < sb_len; i++)
> - buffer[sb_len - i - 1] = wa->address[dm_offset + i];
> - scatterwalk_map_and_copy(buffer, sg, sg_offset, sb_len, 1);
> -
> - sg_offset += sb_len;
> - nbytes -= sb_len;
> + u8 *p, *q;
> +
> + p = wa->address + wa_offset;
> + q = p + len - 1;
> + while (p < q) {
> + *p = *p ^ *q;
> + *q = *p ^ *q;
> + *p = *p ^ *q;
> + p++;
> + q--;
> }
> +
> + ccp_get_dm_area(wa, wa_offset, sg, sg_offset, len);
> }
>
> static void ccp_free_data(struct ccp_data *data, struct ccp_cmd_queue *cmd_q)
> @@ -1294,7 +1278,9 @@ static int ccp_run_rsa_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
> struct ccp_data dst;
> struct ccp_op op;
> unsigned int sb_count, i_len, o_len;
> - int ret;
> + unsigned int dm_offset;
> + int i = 0;

Is "dm_offset" and "i" used anywhere? I don't see them used in this
function...

> + int ret = 0;

No need to change this, is there?

Thanks,
Tom