Devel #28

jschmidb · 2019-06-17T09:19:29Z

Misc. housekeeping and bug fixes: set dynamic ICA flag according to available online cards, don't call ioctl for ECC functions if ECC is not available via cards, enhance icainfo output to three columns, distinguishing between static and dynamic hardware.

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

p-steuer · 2019-06-17T12:58:16Z

src/s390_crypto.c

 			e->flags |= *s390_kdsa_functions[e->id].enabled ? ICA_FLAG_SHW : 0;
 			break;
+		case RSA_ME: /* fall-through */
+		case RSA_CRT:
+			if (any_card_online) {


I think libica RSA only works with CEX-A, no ? So 'any_card_online' would be neccesary but not sufficient.

I found this paragraph in the "Device Drivers, Features, and Commands" book:

For both accelerators and CCA coprocessors, it provides Rivest-Shamir-Adleman
(RSA) encryption and RSA decryption functions using clear keys. RSA operations
are supported in both the modulus-exponent and the Chinese-Remainder Theorem
(CRT) variants for any key size in the range 57 - 4096 bit.

So it looks like the zcrypt device driver can use both types of cards for RSA.

Just tested it: the RSA tests are performed in hw on a CCA coprocessor.

Yes, the zcrypt dd can use both.

p-steuer

approved. ping @hfreude or @ifranzki for second review.

ifranzki · 2019-06-18T15:19:49Z

include/ica_api.h

+	 ICA_PROPERTY_RSA_4096)
+
+#define ICA_PROPERTY_EC_BP			0x00000001 /* Brainpool curves */
+#define ICA_PROPERTY_EC_NIST		0x00000002 /* NIST curves */


Is there a whitespace error ? The NIST curves constant is indented differently than the others in the GitHub diff view....

I will fix this.

ifranzki · 2019-06-18T15:21:02Z

src/s390_crypto.c

+	     msa4_switch, msa5_switch, msa8_switch, trng_switch, msa9_switch,
+		 ecc_via_online_card, any_card_online;
+
+#define CARD_AVAILABLE		0x01


also here it looks like a whitespace error......

will fix it.

ifranzki · 2019-06-18T15:22:25Z

src/s390_crypto.c

+		goto end;
+
+	/* Did we get something like 'CEX5C\n'? */
+	if (strlen(type) != 6 || strstr(type, "CEX") != &type[0])


didn't we say that strstr might not be the best choice here? I would do a strncmp(type, "CEX", 3) instead.

ifranzki · 2019-06-18T15:22:56Z

src/s390_crypto.c

+	while ((direntp = readdir(sysDir)) != NULL) {
+
+		/* Skip entries that are not like "card01", "card02", etc. */
+		if (strstr(direntp->d_name, "card") != &direntp->d_name[0])


strncmp(... ,"card", 4)

hfreude · 2019-06-19T07:50:12Z

src/s390_crypto.c

+}
+
+#define AP_PATH  "/sys/devices/ap"
+#define MAX_DEV_LEN 280


There is a PATH_MAX define in linux/limits.h.

Yes, but it's only 255 bytes long and causes a compiler warning:

s390_crypto.c:401:34: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 239 [-Wformat-truncation=]
401 | snprintf(dev, MAX_DEV_LEN, "%s/%s/type", AP_PATH, direntp->d_name);

The warning disappears when increasing the array to 280 bytes.

hfreude · 2019-06-19T08:00:12Z

src/s390_crypto.c

+{
+	DIR *sysDir;
+	unsigned int ret = 0;
+	char dev[MAX_DEV_LEN] = AP_PATH;


Why the = AP_PATH ?
You use dev later with snprintf(dev, ...) so the assignment is not needed.

Yes, but there's this stmt below which needs the path in dev:

if ((sysDir = opendir(dev)) == NULL)
return 0;

hfreude · 2019-06-19T08:02:27Z

src/s390_crypto.c

+
+		/* Get device type (string like "CEXnT") */
+		snprintf(dev, MAX_DEV_LEN, "%s/%s/type", AP_PATH, direntp->d_name);
+		memset(type, 0, sizeof(type));


the memset here is not needed as you check the returncode from get_device_type()

Yes, that's right.

hfreude · 2019-06-19T08:14:31Z

include/ica_api.h

+#define ICA_PROPERTY_RSA_512		0x00000001
+#define ICA_PROPERTY_RSA_1024		0x00000002
+#define ICA_PROPERTY_RSA_2048		0x00000004
+#define ICA_PROPERTY_RSA_4096		0x00000008


do we really need this differentiation ? I know there was a limit on RSA for CEX2 but as far as I know even CEX3 supports RSA up to 4096 and we do not have any CEX2 support in the device driver any more.

Yes, you are right, today we don't really need it. We could just use ICA_PROPERTY_RSA_ALL. I left the differentiation, because the libica book describes the different RSA key lengths in chapter 5. We may remove the differentiation in the code, but should then also update the book.

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Determine available online cards and check if ECC is supported via CCA coprocessor. Set dynamic flag according to available cards. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

ECC is supported on CEX4C or later. Don't try to perform an ECC function on dynamic hardware if already known that no suitable CCA coprocessor available. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Remove routine to search for cards. Information about available dynamic hardware is now correctly provided via the function list. Add a third column to the icainfo output to show dynamic and static hardware separately. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Introduce and use symbolic constants for already existing AES and RSA properties. Add and use new constants for EC-related properties. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

ifranzki · 2019-06-24T09:28:39Z

src/ica_api.c

+		if (any_card_online)
+			rc = ioctl(adapter_handle, ICARSAMODEXPO, &rb);
+		else
+			rc = 1;


Is there a reason why you return "1" here, an not ENODEV or similar?

I agree, ENODEV is better here. Code changed.

ifranzki · 2019-06-24T09:28:46Z

src/ica_api.c

+		if (any_card_online)
+			rc = ioctl(adapter_handle, ICARSACRT, &rb);
+		else
+			rc = 1;


I agree, ENODEV is better here. Code changed.

This fix applies to RSA, the same technique is already used for ECC also. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Fix typos and comments.

53db569

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

p-steuer reviewed Jun 17, 2019

View reviewed changes

p-steuer approved these changes Jun 18, 2019

View reviewed changes

p-steuer requested review from hfreude and ifranzki June 18, 2019 11:24

jschmidb mentioned this pull request Jun 18, 2019

Fix setting of engine properties. opencryptoki/openssl-ibmca#56

Merged

ifranzki reviewed Jun 18, 2019

View reviewed changes

hfreude reviewed Jun 19, 2019

View reviewed changes

jschmidb added 7 commits June 19, 2019 16:02

Correct formatting.

da4aa52

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Set dynamic ICA flag according to available cards.

7e751c7

Determine available online cards and check if ECC is supported via CCA coprocessor. Set dynamic flag according to available cards. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Don't call ioctl if ECC is not available via cards.

dc907c2

ECC is supported on CEX4C or later. Don't try to perform an ECC function on dynamic hardware if already known that no suitable CCA coprocessor available. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Use symbolic constant for static ICA flag.

09644df

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Introduce symbolic constants for ICA properties.

e79abd5

Introduce and use symbolic constants for already existing AES and RSA properties. Add and use new constants for EC-related properties. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

Update icainfo output example in manpage showing three columns.

892f43c

Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

ifranzki reviewed Jun 24, 2019

View reviewed changes

Call ioctl only if any online card available.

1fff5f2

This fix applies to RSA, the same technique is already used for ECC also. Signed-off-by: Joerg Schmidbauer <jschmidb@de.ibm.com>

ifranzki approved these changes Jun 24, 2019

View reviewed changes

jschmidb merged commit f13678e into opencryptoki:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devel #28

Devel #28

jschmidb commented Jun 17, 2019

p-steuer Jun 17, 2019

jschmidb Jun 17, 2019

jschmidb Jun 17, 2019

hfreude Jun 19, 2019

p-steuer left a comment

ifranzki Jun 18, 2019

jschmidb Jun 19, 2019

ifranzki Jun 18, 2019

jschmidb Jun 19, 2019

ifranzki Jun 18, 2019

jschmidb Jun 19, 2019

ifranzki Jun 18, 2019

jschmidb Jun 19, 2019

hfreude Jun 19, 2019

jschmidb Jun 19, 2019

hfreude Jun 19, 2019

jschmidb Jun 19, 2019

hfreude Jun 19, 2019

jschmidb Jun 19, 2019

hfreude Jun 19, 2019

jschmidb Jun 19, 2019

ifranzki Jun 24, 2019

jschmidb Jun 24, 2019

ifranzki Jun 24, 2019

jschmidb Jun 24, 2019

Devel #28

Devel #28

Conversation

jschmidb commented Jun 17, 2019

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

p-steuer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment