[PATCH] nvme: auth: validate DHCHAP secret before stopping authentication

[PATCH] nvme: auth: validate DHCHAP secret before stopping authentication

[Sreeraj S Kurup]

The driver currently calls nvme_auth_stop() before checking if the new
secret key is valid. If a bad key is passed through sysfs, the key
parser fails and exits early.

Since nvme_auth_stop() was already called, the current authentication
session gets stopped. However, because the parser failed, the work
thread is never restarted. This leaves the NVMe controller stuck in a
stopped state until someone writes a valid key.

Fix this by validating the key before stopping authentication. If the
key is invalid, return an error immediately and leave the running state
machine untouched.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/nvme/host/sysfs.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 02a2490a9ed7..d6512b19752d 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -825,7 +825,9 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
 	struct nvmf_ctrl_options *opts = ctrl->opts;
+	struct nvme_dhchap_key *key = NULL;
 	char *dhchap_secret;
+	int ret;
 
 	if (!ctrl->opts->dhchap_secret)
 		return -EINVAL;
@@ -838,16 +840,17 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
 	if (!dhchap_secret)
 		return -ENOMEM;
 	memcpy(dhchap_secret, buf, count);
-	nvme_auth_stop(ctrl);
+
 	if (strcmp(dhchap_secret, opts->dhchap_secret)) {
-		struct nvme_dhchap_key *key, *host_key;
-		int ret;
+		struct nvme_dhchap_key *host_key;
 
 		ret = nvme_auth_parse_key(dhchap_secret, &key);
 		if (ret) {
 			kfree(dhchap_secret);
 			return ret;
 		}
+
+		nvme_auth_stop(ctrl);
 		kfree(opts->dhchap_secret);
 		opts->dhchap_secret = dhchap_secret;
 		host_key = ctrl->host_key;
@@ -855,8 +858,11 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
 		ctrl->host_key = key;
 		mutex_unlock(&ctrl->dhchap_auth_mutex);
 		nvme_auth_free_key(host_key);
-	} else
+	} else {
+		nvme_auth_stop(ctrl);
 		kfree(dhchap_secret);
+	}
+
 	/* Start re-authentication */
 	dev_info(ctrl->device, "re-authenticating controller\n");
 	queue_work(nvme_wq, &ctrl->dhchap_auth_work);
@@ -883,7 +889,9 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
 	struct nvmf_ctrl_options *opts = ctrl->opts;
+	struct nvme_dhchap_key *key = NULL;
 	char *dhchap_secret;
+	int ret;
 
 	if (!ctrl->opts->dhchap_ctrl_secret)
 		return -EINVAL;
@@ -896,16 +904,17 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
 	if (!dhchap_secret)
 		return -ENOMEM;
 	memcpy(dhchap_secret, buf, count);
-	nvme_auth_stop(ctrl);
+
 	if (strcmp(dhchap_secret, opts->dhchap_ctrl_secret)) {
-		struct nvme_dhchap_key *key, *ctrl_key;
-		int ret;
+		struct nvme_dhchap_key *ctrl_key;
 
 		ret = nvme_auth_parse_key(dhchap_secret, &key);
 		if (ret) {
 			kfree(dhchap_secret);
 			return ret;
 		}
+
+		nvme_auth_stop(ctrl);
 		kfree(opts->dhchap_ctrl_secret);
 		opts->dhchap_ctrl_secret = dhchap_secret;
 		ctrl_key = ctrl->ctrl_key;
@@ -913,8 +922,11 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
 		ctrl->ctrl_key = key;
 		mutex_unlock(&ctrl->dhchap_auth_mutex);
 		nvme_auth_free_key(ctrl_key);
-	} else
+	} else {
+		nvme_auth_stop(ctrl);
 		kfree(dhchap_secret);
+	}
+
 	/* Start re-authentication */
 	dev_info(ctrl->device, "re-authenticating controller\n");
 	queue_work(nvme_wq, &ctrl->dhchap_auth_work);
-- 
2.54.0

Re: [PATCH] nvme: auth: validate DHCHAP secret before stopping authentication

[Hannes Reinecke]

On 9/17/26 10:04 AM, Sreeraj S Kurup wrote:
> The driver currently calls nvme_auth_stop() before checking if the new
> secret key is valid. If a bad key is passed through sysfs, the key
> parser fails and exits early.
> 
> Since nvme_auth_stop() was already called, the current authentication
> session gets stopped. However, because the parser failed, the work
> thread is never restarted. This leaves the NVMe controller stuck in a
> stopped state until someone writes a valid key.
> 
Are you sure?
All what 'nvme_auth_stop()' does is stopping a currently running
authentication. And if someone writes an invalid key it certainly
was his intention to _start_ authentication.
I find it kinda weird to allow an pre-existing authentication to
run while the admin indicates that he wants to start a new one.
So not really sure if this fixes anything.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH] nvme: auth: validate DHCHAP secret before stopping authentication

[Sreeraj S Kurup]

Hi Hannes,

Yes, I agree that stopping the currently running authentication when the admin requests a new one is intentional.

The issue I was pointing out is slightly different: if the new key is invalid, we never actually get to the point where the new authentication can be started again. "nvme_auth_stop()" has already stopped the existing authentication, and the parser returns an error before the restart path is reached.

So the question is whether writing an invalid replacement key should leave the previous authentication stopped, or whether the key should first be parsed/validated and only then replace the existing authentication state.

If the intended semantics are that an invalid key write should not disturb an already running authentication, then moving the validation before "nvme_auth_stop()" would make the operation effectively transactional:

validate new key
    -> error: leave existing authentication unchanged
    -> success: stop old authentication and start with new key

If, however, the intended semantics are that any attempted replacement stops the existing authentication even when the replacement is invalid, then I agree that the current behaviour is intentional and the patch doesn't fix a bug.

My concern was specifically the former behaviour, rather than "nvme_auth_stop()" itself.

Cheers,
Sreeraj

Re: [PATCH] nvme: auth: validate DHCHAP secret before stopping authentication

[Sreeraj S Kurup]

Hi Hannes,

Yes, I agree that stopping the currently running authentication when the admin requests a new one is intentional.

The issue I was pointing out is slightly different: if the new key is invalid, we never actually get to the point where the new authentication can be started again. "nvme_auth_stop()" has already stopped the existing authentication, and the parser returns an error before the restart path is reached.

So the question is whether writing an invalid replacement key should leave the previous authentication stopped, or whether the key should first be parsed/validated and only then replace the existing authentication state.

If the intended semantics are that an invalid key write should not disturb an already running authentication, then moving the validation before "nvme_auth_stop()" would make the operation effectively transactional:

validate new key
    -> error: leave existing authentication unchanged
    -> success: stop old authentication and start with new key

If, however, the intended semantics are that any attempted replacement stops the existing authentication even when the replacement is invalid, then I agree that the current behaviour is intentional and the patch doesn't fix a bug.

My concern was specifically the former behaviour, rather than "nvme_auth_stop()" itself.

Cheers,
Sreeraj