Hi, Great to see those patches coming! After some review, they look to be very well done. Some comments/suggestions below. 1. I strongly recommend to implement DH exponentials reuse (g x mod p / g y mod p as well as g xy mod p) as specified in section 8.13.5.7 "DH-HMAC-CHAP Security Requirements". When I was working on TP 8006 I had a prototype that demonstrated that DH math has quite significant latency, something like (as far as I remember) 30ms for 4K group and few hundreds of ms for 8K group. For single connection it is not a big deal, but imagine AMD EPYC with 128 cores. Since all connections are created sequentially, even with 30 ms per connection time to complete full remote device connection would become 128*30 => almost 4 seconds. With 8K group it might be more than 10 seconds. Users are unlikely going to be happy with this, especially in cases, when connecting multiple of NVMe-oF devices is a part of a server or VM boot sequence. If DH exponential reuse implemented, for all subsequent connections the DH math is excluded, so authentication overhead becomes pretty much negligible. In my prototype I implemented DH exponential reuse as a simple per-host/target cache that keeps DH exponentials (including g xy mod p) for up to 10 seconds. Simple and sufficient. Another, might be ever more significant reason why DH exponential reuse is important is that without it x (or y on the host side) must always be randomly generated each time a new connection is established. Which means, for instance, for 8K groups for each connection 1KB of random bytes must be taken from the random pool. With 128 connections it is now 128KB. Quite a big pressure on the random pool that DH exponential reuse mostly avoids. Those are the 2 reasons why we added this DH exponential reuse sentence in the spec. In the original TP 8006 there was a small informative piece explaining reasonings behind that, but for some reasons it was removed from the final version. 2. What is the status of this code from perspective of stability in face of malicious host behavior? Seems implementation is carefully done, but, for instance, at the first look I was not able to find a code to clean up if host in not acting for too long in the middle of exchange. Other observation is that in nvmet_execute_auth_send() nvmet_check_transfer_len() does not check if tl size is reasonable, i.e., for instance, not 1GB. For sure, we don't want to allow remote hosts to hang or crash target. For instance, because of OOM conditions that happened, because malicious host asked target to allocate too much memory or open to many being authenticated connections in which the host is not going to reply in the middle of exchange. Asking, because don't want to go in my review too far ahead from the author ;) In this regard, it would be great if you add in your test application ability to perform authentication with random parameters and randomly stop responding. Overnight running of such test would give us good degree of confidence that it will always work as expected. Vlad On 7/16/21 2:04 PM, Hannes Reinecke wrote: > Hi all, > > recent updates to the NVMe spec have added definitions for in-band > authentication, and seeing that it provides some real benefit especially > for NVMe-TCP here's an attempt to implement it. > > Tricky bit here is that the specification orients itself on TLS 1.3, > but supports only the FFDHE groups. Which of course the kernel doesn't > support. I've been able to come up with a patch for this, but as this > is my first attempt to fix anything in the crypto area I would invite > people more familiar with these matters to have a look. > > Also note that this is just for in-band authentication. Secure concatenation > (ie starting TLS with the negotiated parameters) is not implemented; one would > need to update the kernel TLS implementation for this, which at this time is > beyond scope. > > As usual, comments and reviews are welcome. > > Hannes Reinecke (11): > crypto: add crypto_has_shash() > crypto: add crypto_has_kpp() > crypto/ffdhe: Finite Field DH Ephemeral Parameters > lib/base64: RFC4648-compliant base64 encoding > nvme: add definitions for NVMe In-Band authentication > nvme: Implement In-Band authentication > nvme-auth: augmented challenge support > nvmet: Parse fabrics commands on all queues > nvmet: Implement basic In-Band Authentication > nvmet-auth: implement support for augmented challenge > nvme: add non-standard ECDH and curve25517 algorithms > > crypto/Kconfig | 8 + > crypto/Makefile | 1 + > crypto/ffdhe_helper.c | 877 +++++++++++++++++ > crypto/kpp.c | 6 + > crypto/shash.c | 6 + > drivers/nvme/host/Kconfig | 11 + > drivers/nvme/host/Makefile | 1 + > drivers/nvme/host/auth.c | 1188 ++++++++++++++++++++++++ > drivers/nvme/host/auth.h | 23 + > drivers/nvme/host/core.c | 77 +- > drivers/nvme/host/fabrics.c | 65 +- > drivers/nvme/host/fabrics.h | 8 + > drivers/nvme/host/nvme.h | 15 + > drivers/nvme/host/trace.c | 32 + > drivers/nvme/target/Kconfig | 10 + > drivers/nvme/target/Makefile | 1 + > drivers/nvme/target/admin-cmd.c | 4 + > drivers/nvme/target/auth.c | 608 ++++++++++++ > drivers/nvme/target/configfs.c | 102 +- > drivers/nvme/target/core.c | 10 + > drivers/nvme/target/fabrics-cmd-auth.c | 472 ++++++++++ > drivers/nvme/target/fabrics-cmd.c | 30 +- > drivers/nvme/target/nvmet.h | 71 ++ > include/crypto/ffdhe.h | 24 + > include/crypto/hash.h | 2 + > include/crypto/kpp.h | 2 + > include/linux/base64.h | 16 + > include/linux/nvme.h | 187 +++- > lib/Makefile | 2 +- > lib/base64.c | 111 +++ > 30 files changed, 3961 insertions(+), 9 deletions(-) > create mode 100644 crypto/ffdhe_helper.c > create mode 100644 drivers/nvme/host/auth.c > create mode 100644 drivers/nvme/host/auth.h > create mode 100644 drivers/nvme/target/auth.c > create mode 100644 drivers/nvme/target/fabrics-cmd-auth.c > create mode 100644 include/crypto/ffdhe.h > create mode 100644 include/linux/base64.h > create mode 100644 lib/base64.c >
