Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH 00/11] nvme: In-band authentication support
To:     Hannes Reinecke <hare@suse.de>, Christoph Hellwig <hch@lst.de>
Cc:     Sagi Grimberg <sagi@grimberg.me>,
        Keith Busch <keith.busch@wdc.com>,
        linux-nvme@lists.infradead.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S . Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org
References: <20210716110428.9727-1-hare@suse.de>
From:   Vladislav Bolkhovitin <vst@vlnb.net>
Message-ID: <66b3b869-02bd-9dee-fadc-8538c6aad57a@vlnb.net>
Date:   Tue, 20 Jul 2021 23:26:37 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20210716110428.9727-1-hare@suse.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>