ACVP Parser changes for LKCM 5.0 by ssrish17 · Pull Request #1 · ssrish17/acvpparser

ssrish17 · 2024-05-24T12:06:28Z

No description provided.

keerthanakalyan

General comment:

Remove log_level logic
Make sure error messages and properly tagged to LOGGER_ERR
Minimize the LOGGER_DEBUG message and remove it in places which are not necessary
Move the declarations to start of the function
Remove functions which are not required (like bin_2_hex_print)
Make sure the buffers allocated are freed during error conditions and in out.
Run static analyzer tool to fix memory issues.

keerthanakalyan · 2024-05-29T04:55:34Z

backends/backend_libkcapi.c

+        switch(acvp_cipher & ACVP_HASHMASK)
+        {
+                case ACVP_SHA1:
+                        if((type & ACVP_DRBGMASK) == ACVP_DRBGHMAC)


Can we move this piece of logic common before?

keerthanakalyan · 2024-05-29T04:58:57Z

backends/backend_libkcapi.c

+                return -EINVAL;
+        }
+
+        char cipher[CIPHERMAXNAME];


Nit: Move all the declarations to the start of the function.

keerthanakalyan · 2024-05-29T05:11:33Z

backends/backend_libkcapi.c

+                goto out;
+        CKINT_LOG(alloc_buf(data->rnd_data_bits_len/8, &data->random), "drbg: data->random buffer could not be allocated\n");
+
+#if log_level==2


Where is log_level defined? I do not see its usage in the existing code for debugging message

keerthanakalyan · 2024-05-29T05:12:43Z

backends/backend_libkcapi.c

+                        ret = kcapi_rng_set_entropy(rng, ent1.buf, ent1.len);
+                        if(ret < 0)
+                        {
+                                logger(LOGGER_DEBUG, "drbg: entropy set failed with error = %d\n", ret);


It should be LOGGER_ERR. Please make sure all the errors are properly classified as LOGGER_ERR

keerthanakalyan · 2024-05-29T05:13:36Z

backends/backend_libkcapi.c

+        }
+        (void)parsed_flags;
+
+        struct kcapi_handle *handle = NULL;


Nit: Move all declarations to the start of function.

keerthanakalyan · 2024-05-29T06:07:30Z

backends/backend_libkcapi.c

+        size_t len = 0;
+        (void)parsed_flags;
+
+        switch(data->cipher) {


Is this logic needed? Looks like only used for logging

keerthanakalyan · 2024-05-29T06:09:50Z

backends/backend_libkcapi.c

+        else
+                ecdsa_get_bufferlen(ACVP_NISTP256, &dlen, &qxlen, &qylen);
+
+        CKINT_LOG(alloc_buf(qxlen, &data->Qx), "ecdsa: Qx could not be allocated\n");


Are these buffers freed anywhere?

keerthanakalyan · 2024-05-29T06:10:07Z

backends/backend_libkcapi.c

+                goto out;
+        }
+
+        CKINT_LOG(alloc_buf(qxlen + qylen, &key), "ecdh: local public Key buffer could not be allocated\n");


where is this buffer getting freed?

keerthanakalyan · 2024-05-29T06:11:07Z

backends/backend_libkcapi.c

+        pk.len = 0;
+        pk.buf = NULL;
+
+        CKINT_LOG(alloc_buf(data->Qx.len + data->Qy.len + 1, &pk), "ecdsa: public key buffer could not be allocated\n");


where is this buffer getting freed?

keerthanakalyan · 2024-05-29T06:11:41Z

backends/backend_libkcapi.c

+        CKINT_LOG(alloc_buf(data->Qx.len + data->Qy.len + 1, &pk), "ecdsa: public key buffer could not be allocated\n");
+        ptr = (&pk)->buf;
+
+        ptr[0] = 0x04;


why is this hardcoded to 0x04? Can you make it a macro with self explanatory name?

…rification rsa_sigver - performs signature verification rsa_siggen - performs signature generation rsa_private_key_ber_encode - perform BER encoding for the RSA private key rsa_public_key_ber_encode - perform BER encoding for the RSA public key Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

decryption and Monte Carlo Test sym_cipher - perform AES cipher selection sym_encrypt - perform AES encryption sym_decrypt - perform AES decryption Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

sha_mac_cipher - performs sha and mac cipher selection sha_generate - generates hash mac_generate - generates message authentication code Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

keerthanakalyan · 2024-06-19T10:22:00Z

backends/backend_libkcapi.c

+static int gcm_encrypt(struct aead_data *data, flags_t parsed_flags)
+{
+        struct kcapi_handle *handle = NULL;
+        char *cipher = "gcm(aes)";


As discussed, please use existing function to return the name, instead of hardcoding.

keerthanakalyan · 2024-06-19T10:22:08Z

backends/backend_libkcapi.c

+        uint8_t *buf = NULL, *newiv = NULL;
+        uint32_t outbuflen = 0, inbuflen = 0;
+
+        uint32_t newivlen = 12;


Please use macro.

keerthanakalyan · 2024-06-19T10:23:40Z

backends/backend_libkcapi.c

+static int gcm_decrypt(struct aead_data *data, flags_t parsed_flags)
+{
+        struct kcapi_handle *handle = NULL;
+        char cipher[] = "gcm(aes)";


As discussed, please use existing function to return the name, instead of hardcoding.

keerthanakalyan · 2024-06-19T10:23:49Z

backends/backend_libkcapi.c

+        uint8_t *buf = NULL, *newiv = NULL;;
+        uint32_t outbuflen = 0, inbuflen = 0;
+        /* Only IV size of 12 bytes is supported by the kernel */
+        uint32_t newivlen = 12;


Please use macro.

keerthanakalyan · 2024-06-19T10:24:04Z

backends/backend_libkcapi.c

+static int ccm_encrypt(struct aead_data *data, flags_t parsed_flags)
+{
+        struct kcapi_handle *handle = NULL;
+        char cipher[] = "ccm(aes)";


As discussed, please use existing function to return the name, instead of hardcoding.

keerthanakalyan · 2024-06-19T10:24:46Z

backends/backend_libkcapi.c

+        uint8_t *outbuf = NULL, *newiv = NULL;
+        uint32_t outbuflen = 0, inbuflen = 0;
+        /* Only IV size of 16 bytes is supported by the kernel */
+        uint32_t newivlen = 16;


please use macro.

keerthanakalyan · 2024-06-19T10:25:10Z

backends/backend_libkcapi.c

+static int ccm_decrypt(struct aead_data *data, flags_t parsed_flags)
+{
+        struct kcapi_handle *handle = NULL;
+        char cipher[] = "ccm(aes)";


As discussed, please use existing function to return the name, instead of hardcoding.

keerthanakalyan · 2024-06-19T10:25:20Z

backends/backend_libkcapi.c

+        uint8_t *buf = NULL, *newiv = NULL;
+        uint32_t outbuflen = 0, inbuflen = 0;
+        /* Only IV size of 16 bytes is supported by the kernel */
+        uint32_t newivlen = 16;


Please use macro.

keerthanakalyan · 2024-06-19T10:26:13Z

backends/backend_libkcapi.c

+
+        if((data->cipher & ACVP_CURVEMASK) == ACVP_NISTP256)
+        {
+                curve = "ecdh-nist-p256";


please use macro for curve, curve_id and ss_key_len values.

keerthanakalyan · 2024-06-19T10:26:32Z

backends/backend_libkcapi.c

+
+        if((data->cipher & ACVP_CURVEMASK) == ACVP_NISTP256)
+        {
+                curve = "ecdh-nist-p256";


please use macro for curve, curve_id and ss_key_len values.

…d signature verification ecdsa_keygen - generates ECDSA keypair ecdsa_keyver - verifies ECDSA keypair ecdsa_sigver - verifies ECDSA signature Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

…and CCM gcm_encrypt - performs GCM encryption gcm_decrypt - performs GCM decryption ccm_encrypt - performs CCM encryption ccm_decrypt - performs CCM decryption Signed-off-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

…ation ecdh_ss_ver - performs ECDH shared secret verification ecdh_ss - performs ECDH shared secret generation Signed-off-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

drbg_cipher - performs DRBG cipher selection drbg_generate - performs DRBG generation Signed-off-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

…age of the deprecated API (#1) * LMS algorithm was added * Removed usage of the deprecated API * Changed compilation lines due to specific OpenSSL layout on Windows + Corrected cryptomb backendns because FIPS CAVP vectors representation was changed Signed-off-by: Stephan Mueller <smueller@chronox.de>

ssrish17 self-assigned this May 24, 2024

keerthanakalyan suggested changes May 29, 2024

View reviewed changes

ssrish17 force-pushed the master branch from 14b1579 to f6ddd51 Compare May 31, 2024 10:57

banvikas and others added 3 commits June 18, 2024 18:20

libkcapi: added support for symmetric AES cipher encryption,

83d342d

decryption and Monte Carlo Test sym_cipher - perform AES cipher selection sym_encrypt - perform AES encryption sym_decrypt - perform AES decryption Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

libkcapi: added support for sha and mac generation

eef787e

sha_mac_cipher - performs sha and mac cipher selection sha_generate - generates hash mac_generate - generates message authentication code Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

ssrish17 force-pushed the master branch from f6ddd51 to d7042c8 Compare June 19, 2024 03:27

keerthanakalyan reviewed Jun 19, 2024

View reviewed changes

banvikas and others added 4 commits June 20, 2024 14:07

libkcapi: added support for ECDSA key generation and verification, an…

1b255b0

…d signature verification ecdsa_keygen - generates ECDSA keypair ecdsa_keyver - verifies ECDSA keypair ecdsa_sigver - verifies ECDSA signature Co-authored-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

libkcapi: added support for ECDH shared secret generation and verific…

4c24a1b

…ation ecdh_ss_ver - performs ECDH shared secret verification ecdh_ss - performs ECDH shared secret generation Signed-off-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

libkcapi: added support for DRBG generation

cdbc476

drbg_cipher - performs DRBG cipher selection drbg_generate - performs DRBG generation Signed-off-by: Srish Srinivasan <srish.srinivasan@broadcom.com>

ssrish17 force-pushed the master branch from d7042c8 to cdbc476 Compare June 20, 2024 09:59

Conversation

ssrish17 commented May 24, 2024

Uh oh!

keerthanakalyan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants