[aes] Add AES-GCM model and DPI #4

nasahlpa · 2024-11-11T13:21:44Z

This PR consists of two commits:

The first commit adds AES-GCM to the AES models
The second commit integrates the AES-GCM model

vogelpi

Thanks @nasahlpa , this looks great! I have added some comments, most of them are really minor.

vogelpi · 2024-11-12T12:05:42Z

hw/ip/aes/model/README.md

@@ -20,7 +20,7 @@ In addition, this directory also contains two example applications.
 2. `aes_modes`:
 - Shows how to interface the OpenSSL/BoringSSL interface functions.
 - Checks the output of BoringSSL/OpenSSL versus expected results.
- Supports ECB, CBC, CTR modes.
+- Supports ECB, CBC, CTR, GCM modes.


also CFB and OFB are supported, I forgot to add that back then.

vogelpi · 2024-11-12T12:10:54Z

hw/ip/aes/model/crypto.h

@@ -14,7 +14,8 @@ typedef enum crypto_mode {
  kCryptoAesCfb = 1 << 2,
  kCryptoAesOfb = 1 << 3,
  kCryptoAesCtr = 1 << 4,
-  kCryptoAesNone = 1 << 5
+  kCryptoAesGcm = 1 << 5,
+  kCryptoAesNone = 1 << 6


We've changed this to not alter the software interface of AES and maintain binary compatibility. kCryptoAesNone is now 6'b11_1111 instead of 7'b100_0000 (initial approach) or 6'b10_0000 (original design, now used for GCM).

I've now changed kCryptoAesNone = 0x3f.

vogelpi · 2024-11-12T12:14:54Z

hw/ip/aes/dv/aes_model_dpi/aes_model_dpi.h

- * @param  mode_i    Cipher mode: 6'b00_0001 = ECB, 6'00_b0010 = CBC,
- *                                6'b00_0100 = CFB, 6'b00_1000 = OFB,
- *                                6'b01_0000 = CTR, 6'b10_0000 = NONE
+ * @param  mode_i    Cipher mode: 7'b000_0001 = ECB, 7'000_b0010 = CBC,
+ *                                7'b000_0100 = CFB, 7'b000_1000 = OFB,
+ *                                7'b001_0000 = CTR, 7'b010_0000 = GCM,
+ *                                7'b010_0000 = NONE


NONE is now 6'b11_1111. I've also left a comment somewhere else about this.

vogelpi · 2024-11-12T12:17:24Z

hw/ip/aes/dv/aes_model_dpi/aes_model_dpi.h

 * @param  data_o    Output data, 1D byte array (open array in SV)
+ * @param  tag_o     Output auth. tag, 2D state matrix (3D packed array in SV)


The output tag should also be a 1D array.

Sorry, we discussed before to use the same data format for this as for the key and IV. I.e. in SV it's a 4 x 32-bit packed 2D array.

Done, @andrea-caforio is testing it and reporting back.

Checked by @andrea-caforio, it is now working. Thanks!

vogelpi · 2024-11-12T12:19:31Z

hw/ip/aes/dv/aes_model_dpi/aes_model_dpi.h

 * @param  data_o    Output data, 2D state matrix (3D packed array in SV)
+ * @param  tag_o     Output auth. tag, 2D state matrix (3D packed array in SV)
 */
 void c_dpi_aes_crypt_block(const unsigned char impl_i, const unsigned char op_i,


There is also a call to this function in hw/top_earlgrey/dv/env/seq_lib/chip_sw_keymgr_sideload_aes_vseq.sv which would need to be adjusted.

Actually, I think we should not change this function at all. Because this is block based anyway. For all the other modes, it's sufficient to pass key, IV, input data to fully define what the output of the next block should be.

This doesn't hold for GCM which also has the internal GHASH state. This is not captured by the IV or any of the other variables.

So when this function is called with GCM mode as argument, it should just throw an error and abort. WDYT?

Done, I am now aborting.

vogelpi · 2024-11-12T12:25:52Z

hw/ip/aes/dv/aes_model_dpi/aes_model_dpi.h

 */
 void c_dpi_aes_crypt_message(unsigned char impl_i, unsigned char op_i,
                             const svBitVecVal *mode_i, const svBitVecVal *iv_i,
                             const svBitVecVal *key_len_i,
                             const svBitVecVal *key_i,
                             const svOpenArrayHandle data_i,
-                             svOpenArrayHandle data_o);
+                             const svOpenArrayHandle aad_i,
+                             const svBitVecVal *tag_i, svOpenArrayHandle data_o,


Nit: line break between tag_i and data_o for readability?

Damn, this is actually enforced by lint.

This commit adds support for the AES-GCM mode to the AES models. For the comparison, test vectors specified by NIST are used. Signed-off-by: Pascal Nasahl <[email protected]>

vogelpi · 2024-11-12T14:14:31Z

Thanks a lot Pascal!

vogelpi · 2024-11-12T14:18:49Z

hw/ip/aes/dv/aes_model_dpi/aes_model_dpi_pkg.sv

-    input  bit       [5:0] mode_i,    // 6'b00_0001 = ECB, 6'00_b0010 = CBC, 6'b00_0100 = CFB,
-                                      // 6'b00_1000 = OFB, 6'b01_0000 = CTR, 6'b10_0000 = NONE
+    input  bit       [6:0] mode_i,    // 7'b000_0001 = ECB, 7'000_b0010 = CBC, 7'b000_0100 = CFB,
+                                      // 7'b000_1000 = OFB, 7'b001_0000 = CTR, 7'b010_0000 = GCM,
+                                      // 7'b100_0000 = NONE


Also this one would need to be changed back.

Ah thanks, I've missed the sv_dpi_aes_crypt_block function. As this is also a block related function, I am removing the tags/aad as well.

This commit enhances the existing AES DPI model with the GCM mode. Currently, only the OpenSSL/BoringSSL reference implementation is supported. Signed-off-by: Pascal Nasahl <[email protected]>

vogelpi

Perfect, thank you both @andrea-caforio and @nasahlpa !

nasahlpa force-pushed the aes-gcm-review branch from fb8ce7a to d27fb7e Compare November 11, 2024 13:41

nasahlpa marked this pull request as ready for review November 12, 2024 07:58

nasahlpa requested a review from vogelpi as a code owner November 12, 2024 07:58

nasahlpa requested a review from andrea-caforio November 12, 2024 07:59

vogelpi reviewed Nov 12, 2024

View reviewed changes

[aes/model] Add AES-GCM to AES models

827d368

This commit adds support for the AES-GCM mode to the AES models. For the comparison, test vectors specified by NIST are used. Signed-off-by: Pascal Nasahl <[email protected]>

nasahlpa force-pushed the aes-gcm-review branch 4 times, most recently from 33d442a to d277855 Compare November 12, 2024 14:11

vogelpi reviewed Nov 12, 2024

View reviewed changes

nasahlpa force-pushed the aes-gcm-review branch 5 times, most recently from 0e1deb8 to 402127b Compare November 12, 2024 15:33

[aes/dv/dpi] Add GCM mode to DPI model

a25cd9a

This commit enhances the existing AES DPI model with the GCM mode. Currently, only the OpenSSL/BoringSSL reference implementation is supported. Signed-off-by: Pascal Nasahl <[email protected]>

nasahlpa force-pushed the aes-gcm-review branch from 402127b to a25cd9a Compare November 12, 2024 15:34

vogelpi approved these changes Nov 12, 2024

View reviewed changes

vogelpi merged commit 1949f42 into vogelpi:aes-gcm-review Nov 12, 2024
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aes] Add AES-GCM model and DPI #4

[aes] Add AES-GCM model and DPI #4

nasahlpa commented Nov 11, 2024

vogelpi left a comment

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi Nov 12, 2024

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi Nov 12, 2024

vogelpi Nov 12, 2024

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi Nov 12, 2024

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi commented Nov 12, 2024

vogelpi Nov 12, 2024

nasahlpa Nov 12, 2024

vogelpi left a comment

		* @param data_o Output data, 1D byte array (open array in SV)
		* @param tag_o Output auth. tag, 2D state matrix (3D packed array in SV)

[aes] Add AES-GCM model and DPI #4

[aes] Add AES-GCM model and DPI #4

Conversation

nasahlpa commented Nov 11, 2024

vogelpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi left a comment

Choose a reason for hiding this comment