[PATCH] nvmet: Free the old negotiation state when restarting authentication

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] nvmet-auth: NULL deref on restart due to stale dhchap_skey_len
--

commit b95a68b07cb2fd80ef9528e69f5467fefe0e42a1
Author: Guixin Liu <kanie at linux.alibaba.com>

nvmet: Free the old negotiation state when restarting authentication

This commit calls nvmet_auth_sq_free() when restarting DH-HMAC-CHAP
negotiation in nvmet_execute_auth_send() so that previously allocated
c1, c2, and skey buffers on the submission queue are freed before a new
round begins.

> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> index 1fe21f2672f10..e68b1c6f2ce5a 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -330,6 +330,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
>  			/* Restart negotiation */
>  			pr_debug("%s: ctrl %d qid %d reset negotiation\n",
>  				 __func__, ctrl->cntlid, req->sq->qid);
> +			nvmet_auth_sq_free(req->sq);

[Severity: Critical]
Can this trigger a NULL pointer dereference in nvmet_auth_host_hash() if
the host restarts negotiation after generating a DH session key and then
sends a reply with dhvlen == 0?

In the first round, when a DH group is configured (ctrl->dh_gid !=
NVME_AUTH_DHGROUP_NULL) and the host sends a reply with a non-zero dhvlen,
nvmet_auth_reply() -> nvmet_auth_ctrl_sesskey() allocates
req->sq->dhchap_skey and sets req->sq->dhchap_skey_len to the hash length.
If that reply fails authentication with NVME_AUTH_DHCHAP_MESSAGE_FAILURE1,
the host can send a new negotiate message to restart authentication.

Calling nvmet_auth_sq_free() here frees sq->dhchap_skey and sets it to
NULL, but leaves sq->dhchap_skey_len non-zero:

drivers/nvme/target/auth.c:nvmet_auth_sq_free() {
	...
	kfree(sq->dhchap_skey);
	sq->dhchap_skey = NULL;
}

If the host then completes the negotiate and challenge steps and sends a
reply with dhvlen == 0 in the second round, nvmet_auth_reply() skips
nvmet_auth_ctrl_sesskey():

drivers/nvme/target/fabrics-cmd-auth.c:nvmet_auth_reply() {
	...
	if (dhvlen) {
		if (!ctrl->dh_tfm)
			return NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
		if (nvmet_auth_ctrl_sesskey(req, data->rval + 2 * data->hl,
					    dhvlen) < 0)
			return NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE;
	}
	...
	if (nvmet_auth_host_hash(req, response, data->hl) < 0) {
}

nvmet_auth_host_hash() then passes the NULL req->sq->dhchap_skey along with
the stale non-zero req->sq->dhchap_skey_len to
nvme_auth_augmented_challenge():

drivers/nvme/target/auth.c:nvmet_auth_host_hash() {
	...
	if (ctrl->dh_gid != NVME_AUTH_DHGROUP_NULL) {
		challenge = kmalloc(shash_len, GFP_KERNEL);
		if (!challenge) {
			ret = -ENOMEM;
			goto out_free_response;
		}
		ret = nvme_auth_augmented_challenge(ctrl->shash_id,
						    req->sq->dhchap_skey,
						    req->sq->dhchap_skey_len,
						    req->sq->dhchap_c1,
						    challenge, shash_len);
}

Because skey_len is non-zero while skey is NULL,
nvme_auth_augmented_challenge() -> nvme_auth_hmac() ->
nvme_auth_hmac_init() dereferences NULL when preparing the HMAC key.

Should nvmet_auth_sq_free() also reset sq->dhchap_skey_len = 0 (or should
nvmet_auth_reply() require dhvlen != 0 when ctrl->dh_gid !=
NVME_AUTH_DHGROUP_NULL)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028064345.544504-1-kanie@linux.alibaba.com?part=1