[PATCHv2 0/2] nvme: validate CNTLID

[PATCHv2 0/2] nvme: validate CNTLID

[Hannes Reinecke]

Hi all,

here are two patches to validate correct CNTLID information.
A controller might violate the constrain the each CNTLID has to
be unique within a subsystem, which then would cause the host
to crash.
So these patches prevent this situation by validate the CNTLID
and not use the cntlid as part of the device name.

As usual, comments and reviews are welcome.

Changes to v1:
- split cntlid validation into a separate helper and moved to
  nvme_init_subsystem()

Hannes Reinecke (2):
  nvme-multipath: avoid crash on invalid subsystem cntlid enumeration
  nvme: validate cntlid during controller initialisation

 drivers/nvme/host/core.c      | 27 +++++++++++++++++++++++++++
 drivers/nvme/host/multipath.c |  2 +-
 2 files changed, 28 insertions(+), 1 deletion(-)

-- 
2.16.4

[PATCHv2 1/2] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Hannes Reinecke]

A process holding an open reference to a removed disk prevents it
from completing deletion, so its name continues to exist. A subsequent
gendisk creation may have the same cntlid which risks collision when
using that for the name. Use the unique ctrl->instance instead.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5c9429d41120..499acf07d61a 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -31,7 +31,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 		sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
 	} else if (ns->head->disk) {
 		sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
-				ctrl->cntlid, ns->head->instance);
+				ctrl->instance, ns->head->instance);
 		*flags = GENHD_FL_HIDDEN;
 	} else {
 		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
-- 
2.16.4

[PATCHv2 2/2] nvme: validate cntlid during controller initialisation

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.com>

The CNTLID value is required to be unique, and we do rely on this
for correct operation. So reject any controller for which a non-unique
CNTLID has been detected.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cd16d98d1f1a..b0396135f097 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2358,6 +2358,25 @@ static int nvme_active_ctrls(struct nvme_subsystem *subsys)
 	return count;
 }
 
+static bool nvme_duplicate_cntlid(struct nvme_subsystem *subsys,
+				  struct nvme_ctrl *ctrl)
+{
+	struct nvme_ctrl *tmp;
+	bool ret = false;
+
+	mutex_lock(&subsys->lock);
+	list_for_each_entry(tmp, &subsys->ctrls, subsys_entry) {
+		if (tmp == ctrl)
+			continue;
+		if (tmp->cntlid == ctrl->cntlid) {
+			ret = true;
+			break;
+		}
+	}
+	mutex_unlock(&subsys->lock);
+	return ret;
+}
+
 static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
 	struct nvme_subsystem *subsys, *found;
@@ -2408,6 +2427,14 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 			ret = -EINVAL;
 			goto out_unlock;
 		}
+		if (nvme_duplicate_cntlid(found, ctrl)) {
+			dev_err(ctrl->device,
+				"Duplicate cntlid %u, rejecting\n",
+				ctrl->cntlid);
+			nvme_put_subsystem(found);
+			ret = -EINVAL;
+			goto out_unlock;
+		}
 
 		__nvme_release_subsystem(subsys);
 		subsys = found;
-- 
2.16.4

[PATCHv2 2/2] nvme: validate cntlid during controller initialisation

[Christoph Hellwig]

On Fri, May 03, 2019@02:26:42PM +0200, Hannes Reinecke wrote:
> +static bool nvme_duplicate_cntlid(struct nvme_subsystem *subsys,
> +				  struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_ctrl *tmp;
> +	bool ret = false;
> +
> +	mutex_lock(&subsys->lock);
> +	list_for_each_entry(tmp, &subsys->ctrls, subsys_entry) {
> +		if (tmp == ctrl)
> +			continue;
> +		if (tmp->cntlid == ctrl->cntlid) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&subsys->lock);
> +	return ret;
> +}

We'll need to do the duplicate check under the same subsys->lock
critical section as adding the controller to the list, otherwise it
will be racy.