opencryptoki / libica

Crypto library for s390x.
Other
7 stars 18 forks source link

adapter_handle_test fails when CEX adapter is available #126

Closed sharkcz closed 5 months ago

sharkcz commented 5 months ago

If I read the test right, then the adapter_handle_test expects that opening /dev/z90crypt device will return 3 as the file descriptor. But it doesn't seem to be the case when a CEX adapter is available in the system as something does some kind of initialization and grabs the fd 3. The output is following

[root@s390x-kvm-121 libica]# LD_LIBRARY_PATH=./src/.libs ./test/adapter_handle_test -v
ica_open_adapter: file descriptor value is greater than 3 (current 4).

strace than reveals

[root@s390x-kvm-121 libica]# LD_LIBRARY_PATH=./src/.libs strace ./test/adapter_handle_test -v
execve("./test/adapter_handle_test", ["./test/adapter_handle_test", "-v"], 0x3ffdb4f9308 /* 44 vars */) = 0
brk(NULL)                               = 0x1cd5000
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3ff84ef9000
...
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3ff84ef7000
set_tid_address(0x3ff84ef8350)          = 58839
set_robust_list(0x3ff84ef8360, 24)      = 0
rseq(0x3ff84ef89a0, 0x20, 0, 0xb2ff0fff) = 0
mprotect(0x3ff848e3000, 12288, PROT_READ) = 0
mprotect(0x3ff8469b000, 4096, PROT_READ) = 0
mprotect(0x3ff84caa000, 348160, PROT_READ) = 0
mprotect(0x3ff84e33000, 4096, PROT_READ) = 0
mprotect(0x1003000, 4096, PROT_READ)    = 0
mprotect(0x3ff84eac000, 8192, PROT_READ) = 0
prlimit64(0, RLIMIT_STACK, NULL, {rlim_cur=8192*1024, rlim_max=RLIM64_INFINITY}) = 0
munmap(0x3ff84d80000, 17579)            = 0
openat(AT_FDCWD, "/dev/z90crypt", O_RDWR|O_CLOEXEC) = 3
ioctl(3, ICARSAMODEXPO, 0x3ffd64f9730)  = 0
getrandom("\xa8\x5b\x20\x4c\x1c\xda\x60\x67", 8, GRND_NONBLOCK) = 8
brk(NULL)                               = 0x1cd5000
brk(0x1cf6000)                          = 0x1cf6000
geteuid()                               = 0
...
openat(AT_FDCWD, "/dev/prandom", O_RDONLY) = 4
fstat(4, {st_mode=S_IFCHR|0644, st_rdev=makedev(0xa, 0x79), ...}) = 0
ioctl(4, TCGETS, 0x3ffd64f9168)         = -1 ENOTTY (Inappropriate ioctl for device)
read(4, "\206V\324.N\350A?\10\3548\200\207\354\271\345\201Z!/\330K&\361\360O\347\25G9\342o"..., 4096) = 4096
close(4)                                = 0
openat(AT_FDCWD, "/udev/z90crypt", O_RDWR) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/dev/z90crypt", O_RDWR) = 4
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0), ...}) = 0
write(1, "ica_open_adapter: file descripto"..., 71ica_open_adapter: file descriptor value is greater than 3 (current 4).
) = 71
munmap(0x3ff84d80000, 3680)             = 0
close(3)                                = 0
exit_group(1)                           = ?
+++ exited with 1 +++

The system is Fedora 40 running in a VM on a z16 with

[root@s390x-kvm-121 libica]# lszcrypt
CARD.DOM TYPE  MODE        STATUS     REQUESTS
----------------------------------------------
00       CEX8C CCA-Coproc  online         3664
00.0038  CEX8C CCA-Coproc  online         3664

The ioctl(3, ICARSAMODEXPO,...) call fails when there is no CEX adapter and and fd 3 is "returned back".

ifranzki commented 5 months ago

Strange. further up it does open it at fd 3: openat(AT_FDCWD, "/dev/z90crypt", O_RDWR|O_CLOEXEC) = 3 but then it opens it again at fd 4 openat(AT_FDCWD, "/dev/z90crypt", O_RDWR) = 4

sharkcz commented 5 months ago

Yup, the first open() is some kind of initialization, in the non-CEX case the getrandom() is called a little bit later. The second open() of from ica_open_adapter().

ifranzki commented 5 months ago

Ahhh: You probably have an OpenSSL 3.3.0 running. There it also opens /dev/z90crypt for its RSA acceleration: https://github.com/openssl/openssl/blob/bd73e1e62c4103e0faffb79cb3d34a2a92a95439/crypto/s390xcap.c#L219

This is new in OpenSSL 3.3 and wasn't there before.

You can set environment variable OPENSSL_s390xcap=nocex to disable that: https://www.openssl.org/docs/manmaster/man3/OPENSSL_s390xcap.html

I guess we need to adjust the testcases in libica to account for that.

sharkcz commented 5 months ago

This is with openssl-3.2.1-2.fc40.s390x. The non-CEX case contains

...
prlimit64(0, RLIMIT_STACK, NULL, {rlim_cur=8192*1024, rlim_max=RLIM64_INFINITY}) = 0
munmap(0x3ff8cd00000, 13127)            = 0
openat(AT_FDCWD, "/dev/z90crypt", O_RDWR|O_CLOEXEC) = 3
ioctl(3, ICARSAMODEXPO, 0x3ffc9ef9bf0)  = -1 ENODEV (No such device)
close(3)                                = 0
geteuid()                               = 0
openat(AT_FDCWD, "/dev/shm/icastats_0", O_RDWR|O_NOFOLLOW|O_CLOEXEC) = 3
...
ifranzki commented 5 months ago

Ah right, commit https://github.com/openssl/openssl/commit/79040cf29e011c21789563d74da626b7465a0540 went into OpenSSL 3.2 already.

ifranzki commented 5 months ago

The whole adapter_handle_test is a bit bogus in my opinion.... I guess its a bad idea in general to expect a certain file descriptor number. Maybe we should just drop that test....

jschmidb commented 5 months ago

I agree. With commit https://github.com/opencryptoki/libica/commit/0a3ad94913979cbbbded20b49155d4d3f00c5032 we even removed the check if an ioctl would succeed. So yes, why not dropping this test at all.

sharkcz commented 5 months ago

@kkaarreell, FYI

ifranzki commented 5 months ago

@holger-dengler added this test in 2022 with the following commit message:

Add a unit test for ica_open_adapter(). It is expected, that a process,
which calls this API call gets a file handle to the adapter, which should
not be greater than 3. If this test fails, the library initialization
opens a file descriptor, but not closing it in the end.

I am not sure where the "not greater than 3" comes from.... I guess its just to test that library initialization does not unexpectedly open additional file descriptors which it does not close. So in that sense the test makes some sense.

However, part of libica library initialization is also that OpenSSL (and maybe other dependent libraries as well) gets initialized. It is quite hard to predict what those libraries do in regards of file descriptions. It might be totally OK that any of these libraries opens file descriptors, like OpenSSL >= 3.2 does for performing RSA acceleration with /dev/z90crypt.

Unfortunately, we can't really detect if the additional file descriptors were opened by libica itself, or by a library that gets loaded as well. Any additional file descriptors opened by libica itself should be subject to make this test fail, while all others should not make it fail...

holger-dengler commented 5 months ago

The problem is, that openssh has a hard limit for open file descriptors in their sandbox. If with openssl3.32 the /dev/z90crypt device node is opened twice (1st time by openssl and 2nd time by libica itself), we'll get problems in the sandbox.

The idea for the test was, to get the adapter-handle from libica and check its fd number. As the test case is a "normal" process with 0/1/2 or stdin/stdout/stderr, the next free fd value must be 3. Therefore we have a hard check of the value.

In context of libica, the new support of crypto-card in openssl does not make any sense, because libica is redirecting the workload by itself to the adapters. We should disable the usage of crypto adapters in openssl if it is used by libica.

(cex support is available since openssl3.2, not 3.3)

ifranzki commented 5 months ago

Well, on most distros OpenSSH has a special patch to allow /dev/z90crypt to survive the sandbox: https://src.fedoraproject.org/rpms/openssh/blob/main/f/openssh-7.2p2-s390-closefrom.patch If I see this right, then it won't matter how often /dev/z90crypt is open, it will allow to survive for all of them.

holger-dengler commented 5 months ago

And what is the benefit of having the device node opened twice?

ifranzki commented 5 months ago

And what is the benefit of having the device node opened twice?

There is no benefit, its just that two independent libraries open it themselves, without knowing from each other. So we have to live with that, whether we like it or not.

The real question is: Does it hurt if it is opened twice, and I would say no, it does not hurt.

holger-dengler commented 5 months ago

I would personally prefer to disable the (unused) cex support in openssl by extending the environment variable OPENSSL_s390xcap in libica init with nocex. But if you prefer to drop the testcase instead, I'm also fine with that.

ifranzki commented 5 months ago

extending the environment variable OPENSSL_s390xcap in libica init with nocex

I thought about that, too, but I guess it is too late inside the libica library constructor. Since libica is linked against OpenSSL, the OpenSSL library will be loaded and initialized prior to running the libica library constructor.

Also, I am not sure if we always want to disable OpenSSL's CEX usage when libica is used.... I guess that would be a bit too pervasive.

We could set OPENSSL_s390xcap=nocex in the adapter_handle_test testcase, but there we have the same problem, its too late. adapter_handle_test is linked against the libica library, so the libica constructor runs before the main function is called. We would need to load libica via dlopen() instead to set the environment variable before loading libica.

The other problem is that older OpenSSL version do not support the nocex keyword. Not sure what happens when this is used....

holger-dengler commented 5 months ago

what about this hack?

diff --git a/test/adapter_handle_test.c b/test/adapter_handle_test.c
index e451eee..481bf21 100644
--- a/test/adapter_handle_test.c
+++ b/test/adapter_handle_test.c
@@ -8,15 +8,21 @@

 #include <stdio.h>
 #include <errno.h>
+#include <openssl/opensslv.h>

 #include "ica_api.h"
 #include "testcase.h"

 int main(int argc, char **argv)
 {
-       ica_adapter_handle_t adapter_handle;
+       ica_adapter_handle_t adapter_handle, max_handle = 3;
        int rc;

+#if OPENSSL_VERSION_PREREQ(3, 2)
+       /* openssl3.2 internally opens cex device node */
+       max_handle = 4;
+#endif
+
        set_verbosity(argc, argv);

        rc = ica_open_adapter(&adapter_handle);
@@ -25,9 +31,9 @@ int main(int argc, char **argv)
                return TEST_FAIL;
        }

-       if (adapter_handle > 3) {
-               V_(printf("ica_open_adapter: file descriptor value is greater than 3 (current %d).\n",
-                         adapter_handle));
+       if (adapter_handle > max_handle) {
+               V_(printf("ica_open_adapter: file descriptor value is greater than %d (current %d).\n",
+                         max_handle, adapter_handle));
                return TEST_FAIL;
        }
ifranzki commented 5 months ago

That would work if we assume that the test runs using the same OpenSSL version that it was compiled against.

What is not covered is that if it runs on a system that has no suitable CEX adapters, then you would still get fd = 3. This passes the test, but you would not see if libica itself would open another file descriptor (whihc is what the test is all about)......

I also thought about checking the file descriptors > 2 via /proc/self/fd/<n> and verify that they are only /dev/z90crypt fds. But still there could be other legit file descriptors being opened by other libraries....

holger-dengler commented 5 months ago

If there is no adapter in the system, ica_open_adapter() will return -1, right?

ifranzki commented 5 months ago

If there is no adapter in the system, ica_open_adapter() will return -1, right?

No it will still open the device, but any IOCLT sent to it will return ENODEV.

holger-dengler commented 5 months ago

If /dev/z90crypt exists, libica and openssl will get a valid fd. If the device node does not exist, both will not get a -1.

ifranzki commented 5 months ago

No, OpenSSL does a probe by sending an IOCTL to it and closes the fd if it gets ENODEV.

jschmidb commented 5 months ago

Guys, what about a more "heuristic" test: we check the different cases as Holger suggests above, but we don't fail in any case. We just provide the info. Perhaps it's of some value for someone checking the test result ...

sharkcz commented 5 months ago

Works for me, the upper level test driver can then expect 4, when CEX is present, or 3, when it's not.

holger-dengler commented 5 months ago

@sharkcz can you please check, if this fix works for you?