On 7/19/21 11:21 AM, Sagi Grimberg wrote:

On 7/16/21 4:04 AM, Hannes Reinecke wrote:

quoted

Implement support for augmented challenge using FFDHE groups.

Please some more info for the change log...

quoted

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
  drivers/nvme/host/auth.c | 403 +++++++++++++++++++++++++++++++++++----
  1 file changed, 371 insertions(+), 32 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 448a3adebea6..754343aced19 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c

[ .. ]

quoted

@@ -290,10 +382,24 @@ static int nvme_auth_dhchap_challenge(struct

nvme_ctrl *ctrl,
          return -EPROTO;
      }
      if (data->dhgid != NVME_AUTH_DHCHAP_DHGROUP_NULL) {
-        chap->status = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE;
-        return -EPROTO;
-    }
-    if (data->dhgid == NVME_AUTH_DHCHAP_DHGROUP_NULL && data->dhvlen
!= 0) {
+        if (data->dhvlen == 0) {
+            dev_warn(ctrl->device,
+                 "qid %d: DH-HMAC-CHAP: empty DH value\n",
+                 chap->qid);
+            chap->status = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE;
+            return -EPROTO;
+        }
+        chap->dh_tfm = crypto_alloc_kpp(gid_name, 0, 0);
+        if (IS_ERR(chap->dh_tfm)) {
+            dev_warn(ctrl->device,
+                 "qid %d: DH-HMAC-CHAP: failed to initialize %s\n",
+                 chap->qid, gid_name);
+            chap->status = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE;
+            chap->dh_tfm = NULL;
+            return -EPROTO;

Why not propogate the error?

Will be doing so.

quoted

+        }
+        chap->dhgroup_id = data->dhgid;
+    } else if (data->dhvlen != 0) {
          dev_warn(ctrl->device,
               "qid %d: DH-HMAC-CHAP: invalid DH value for NULL DH\n",
              chap->qid);

@@ -313,6 +419,16 @@ static int nvme_auth_dhchap_challenge(struct

nvme_ctrl *ctrl,
      chap->hash_len = data->hl;
      chap->s1 = le32_to_cpu(data->seqnum);
      memcpy(chap->c1, data->cval, chap->hash_len);
+    if (data->dhvlen) {
+        chap->ctrl_key = kmalloc(data->dhvlen, GFP_KERNEL);
+        if (!chap->ctrl_key)
+            return -ENOMEM;
+        chap->ctrl_key_len = data->dhvlen;
+        memcpy(chap->ctrl_key, data->cval + chap->hash_len,
+               data->dhvlen);
+        dev_dbg(ctrl->device, "ctrl public key %*ph\n",
+             (int)chap->ctrl_key_len, chap->ctrl_key);
+    }
        return 0;
  }

@@ -353,10 +469,13 @@ static int nvme_auth_dhchap_reply(struct

nvme_ctrl *ctrl,
          memcpy(data->rval + chap->hash_len, chap->c2,
                 chap->hash_len);
      }
-    if (chap->host_key_len)
+    if (chap->host_key_len) {
+        dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",
+            __func__, chap->qid,
+            chap->host_key_len, chap->host_key);
          memcpy(data->rval + 2 * chap->hash_len, chap->host_key,
                 chap->host_key_len);
-
+    }

Is this change only adding the debug print?

Might. I'll check.

quoted

      return size;
  }
  @@ -440,23 +559,10 @@ static int nvme_auth_dhchap_failure2(struct
nvme_ctrl *ctrl,
  int nvme_auth_select_hash(struct nvme_ctrl *ctrl,
                struct nvme_dhchap_context *chap)
  {
-    char *hash_name;
+    const char *hash_name, *digest_name;
      int ret;
  -    switch (chap->hash_id) {
-    case NVME_AUTH_DHCHAP_HASH_SHA256:
-        hash_name = "hmac(sha256)";
-        break;
-    case NVME_AUTH_DHCHAP_HASH_SHA384:
-        hash_name = "hmac(sha384)";
-        break;
-    case NVME_AUTH_DHCHAP_HASH_SHA512:
-        hash_name = "hmac(sha512)";
-        break;
-    default:
-        hash_name = NULL;
-        break;
-    }
+    hash_name = nvme_auth_hmac_name(chap->hash_id);
      if (!hash_name) {
          chap->status = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
          return -EPROTO;

@@ -468,26 +574,100 @@ int nvme_auth_select_hash(struct nvme_ctrl *ctrl,

          chap->shash_tfm = NULL;
          return -EPROTO;
      }
+    digest_name = nvme_auth_digest_name(chap->hash_id);
+    if (!digest_name) {
+        crypto_free_shash(chap->shash_tfm);
+        chap->shash_tfm = NULL;
+        return -EPROTO;
+    }
+    chap->digest_tfm = crypto_alloc_shash(digest_name, 0, 0);
+    if (IS_ERR(chap->digest_tfm)) {
+        chap->status = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
+        crypto_free_shash(chap->shash_tfm);
+        chap->shash_tfm = NULL;
+        chap->digest_tfm = NULL;
+        return -EPROTO;
+    }
      if (!chap->key) {
          dev_warn(ctrl->device, "qid %d: cannot select hash, no key\n",
               chap->qid);
          chap->status = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
+        crypto_free_shash(chap->digest_tfm);
          crypto_free_shash(chap->shash_tfm);
          chap->shash_tfm = NULL;
+        chap->digest_tfm = NULL;
          return -EINVAL;

Please have a structured goto targets in reverse order, this repeated
cleanup is a mess...

Already done.

quoted

      }
      ret = crypto_shash_setkey(chap->shash_tfm, chap->key,
chap->key_len);
      if (ret) {
          chap->status = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
+        crypto_free_shash(chap->digest_tfm);
          crypto_free_shash(chap->shash_tfm);
          chap->shash_tfm = NULL;
+        chap->digest_tfm = NULL;
          return ret;
      }
-    dev_info(ctrl->device, "qid %d: DH-HMAC_CHAP: selected hash %s\n",
-         chap->qid, hash_name);
+    dev_dbg(ctrl->device, "qid %d: DH-HMAC_CHAP: selected hash %s\n",
+        chap->qid, hash_name);
      return 0;
  }
  +static int nvme_auth_augmented_challenge(struct nvme_dhchap_context
*chap,
+                     u8 *challenge, u8 *aug)
+{
+    struct crypto_shash *tfm;
+    struct shash_desc *desc;
+    u8 *hashed_key;
+    const char *hash_name;
+    int ret;
+
+    hashed_key = kmalloc(chap->hash_len, GFP_KERNEL);
+    if (!hashed_key)
+        return -ENOMEM;
+
+    ret = crypto_shash_tfm_digest(chap->digest_tfm, chap->sess_key,
+                      chap->sess_key_len, hashed_key);
+    if (ret < 0) {
+        pr_debug("failed to hash session key, err %d\n", ret);
+        kfree(hashed_key);

Same here...

quoted

+        return ret;
+    }

Spaces between if conditions please?

quoted

+    hash_name = crypto_shash_alg_name(chap->shash_tfm);
+    if (!hash_name) {
+        pr_debug("Invalid hash algoritm\n");
+        return -EINVAL;
+    }
+    tfm = crypto_alloc_shash(hash_name, 0, 0);
+    if (IS_ERR(tfm)) {
+        ret = PTR_ERR(tfm);
+        goto out_free_key;
+    }
+    desc = kmalloc(sizeof(struct shash_desc) +
crypto_shash_descsize(tfm),
+               GFP_KERNEL);
+    if (!desc) {
+        ret = -ENOMEM;
+        goto out_free_hash;
+    }
+    desc->tfm = tfm;
+
+    ret = crypto_shash_setkey(tfm, hashed_key, chap->hash_len);
+    if (ret)
+        goto out_free_desc;
+    ret = crypto_shash_init(desc);
+    if (ret)
+        goto out_free_desc;
+    crypto_shash_update(desc, challenge, chap->hash_len);
+    crypto_shash_final(desc, aug);
+
+out_free_desc:
+    kfree_sensitive(desc);
+out_free_hash:
+    crypto_free_shash(tfm);
+out_free_key:
+    kfree(hashed_key);
+    return ret;
+}
+
  static int nvme_auth_dhchap_host_response(struct nvme_ctrl *ctrl,
                        struct nvme_dhchap_context *chap)
  {

@@ -497,6 +677,16 @@ static int nvme_auth_dhchap_host_response(struct

nvme_ctrl *ctrl,
        dev_dbg(ctrl->device, "%s: qid %d host response seq %d
transaction %d\n",
          __func__, chap->qid, chap->s1, chap->transaction);
+    if (chap->dh_tfm) {
+        challenge = kmalloc(chap->hash_len, GFP_KERNEL);
+        if (!challenge) {
+            ret = -ENOMEM;
+            goto out;
+        }
+        ret = nvme_auth_augmented_challenge(chap, chap->c1, challenge);
+        if (ret)
+            goto out;
+    }
      shash->tfm = chap->shash_tfm;
      ret = crypto_shash_init(shash);
      if (ret)

@@ -532,6 +722,8 @@ static int nvme_auth_dhchap_host_response(struct

nvme_ctrl *ctrl,
          goto out;
      ret = crypto_shash_final(shash, chap->response);
  out:
+    if (challenge != chap->c1)
+        kfree(challenge);
      return ret;
  }
  @@ -542,6 +734,17 @@ static int
nvme_auth_dhchap_ctrl_response(struct nvme_ctrl *ctrl,
      u8 buf[4], *challenge = chap->c2;
      int ret;
  +    if (chap->dh_tfm) {
+        challenge = kmalloc(chap->hash_len, GFP_KERNEL);
+        if (!challenge) {
+            ret = -ENOMEM;
+            goto out;
+        }
+        ret = nvme_auth_augmented_challenge(chap, chap->c2,
+                            challenge);
+        if (ret)
+            goto out;
+    }
      dev_dbg(ctrl->device, "%s: qid %d host response seq %d
transaction %d\n",
          __func__, chap->qid, chap->s2, chap->transaction);
      dev_dbg(ctrl->device, "%s: qid %d challenge %*ph\n",

@@ -585,6 +788,8 @@ static int nvme_auth_dhchap_ctrl_response(struct

nvme_ctrl *ctrl,
          goto out;
      ret = crypto_shash_final(shash, chap->response);
  out:
+    if (challenge != chap->c2)
+        kfree(challenge);

Just free ?! what about failing?

This is not an error condition, but rather the case when we need to
construct an augmented challenge; in that case we'll allocate a
temporary buffer in 'challenge', and copy it over into 'c2'.

quoted

      return ret;
  }
  @@ -644,10 +849,134 @@ int nvme_auth_generate_key(struct nvme_ctrl
*ctrl,
      return 0;
  }
  +static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
+                    struct nvme_dhchap_context *chap)
+{
+    struct kpp_request *req;
+    struct crypto_wait wait;
+    struct scatterlist src, dst;
+    u8 *pkey;
+    int ret, pkey_len;
+
+    if (chap->dhgroup_id == NVME_AUTH_DHCHAP_DHGROUP_2048 ||
+        chap->dhgroup_id == NVME_AUTH_DHCHAP_DHGROUP_3072 ||
+        chap->dhgroup_id == NVME_AUTH_DHCHAP_DHGROUP_4096 ||
+        chap->dhgroup_id == NVME_AUTH_DHCHAP_DHGROUP_6144 ||
+        chap->dhgroup_id == NVME_AUTH_DHCHAP_DHGROUP_8192) {
+        struct dh p = {0};
+        int pubkey_size =
nvme_auth_dhgroup_pubkey_size(chap->dhgroup_id);
+
+        ret = crypto_ffdhe_params(&p, pubkey_size << 3);
+        if (ret) {
+            dev_dbg(ctrl->device,
+                "failed to generate ffdhe params, error %d\n",
+                ret);
+            return ret;
+        }
+        p.key = chap->key;
+        p.key_size = chap->key_len;
+
+        pkey_len = crypto_dh_key_len(&p);
+        pkey = kzalloc(pkey_len, GFP_KERNEL);
+
+        get_random_bytes(pkey, pkey_len);
+        ret = crypto_dh_encode_key(pkey, pkey_len, &p);
+        if (ret) {
+            dev_dbg(ctrl->device,
+                "failed to encode pkey, error %d\n", ret);
+            kfree(pkey);
+            return ret;
+        }
+        chap->host_key_len = pubkey_size;
+        chap->sess_key_len = pubkey_size;
+    } else {
+        dev_warn(ctrl->device, "Invalid DH group id %d\n",
+             chap->dhgroup_id);
+        chap->status = NVME_AUTH_DHCHAP_FAILURE_INVALID_PAYLOAD;
+        return -EINVAL;
+    }
+
+    ret = crypto_kpp_set_secret(chap->dh_tfm, pkey, pkey_len);
+    if (ret) {
+        dev_dbg(ctrl->dev, "failed to set secret, error %d\n", ret);
+        kfree(pkey);
+        return ret;
+    }
+    req = kpp_request_alloc(chap->dh_tfm, GFP_KERNEL);
+    if (!req) {
+        ret = -ENOMEM;
+        goto out_free_exp;
+    }
+
+    chap->host_key = kzalloc(chap->host_key_len, GFP_KERNEL);
+    if (!chap->host_key) {
+        ret = -ENOMEM;
+        goto out_free_req;
+    }
+    crypto_init_wait(&wait);
+    kpp_request_set_input(req, NULL, 0);
+    sg_init_one(&dst, chap->host_key, chap->host_key_len);
+    kpp_request_set_output(req, &dst, chap->host_key_len);
+    kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+                 crypto_req_done, &wait);
+
+    ret = crypto_wait_req(crypto_kpp_generate_public_key(req), &wait);
+    if (ret == -EOVERFLOW) {
+        dev_dbg(ctrl->dev,
+            "public key buffer too small, wants %d is %d\n",
+            crypto_kpp_maxsize(chap->dh_tfm), chap->host_key_len);
+        goto out_free_host;

Is this a specific retcode of intereset? Why did you specifically add
special casing here?

Because that's the specific error code from the DH code, indicating that
the length isn't correct. And I needed that during development of the
FFDHE code.
But yeah, it can be removed.


Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme