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

[Guixin Liu <kanie@linux.alibaba.com>]

在 2026/10/28 14:59, sashiko-bot@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@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 is right, I should set dhchap_skey_len to zero in nvmet_auth_sq_free().
Changed in v2.
>