nvme-rdma: Don't leak uninitialized memory in connect request private data

From: Roland Dreier <roland at purestorage.com>

If a target address does not parse as IPv4, try parsing it as IPv6 (including handling '%<scope-id>' suffixes for link-local addresses).

Signed-off-by Roland Dreier <roland at purestorage.com>

Quoted 5 lines

From: Roland Dreier <roland at purestorage.com>

If a target address does not parse as IPv4, try parsing it as IPv6 (including handling '%<scope-id>' suffixes for link-local addresses).

Quoted 1 line

Signed-off-by Roland Dreier <roland at purestorage.com>

Nice! thanks Roland,

testing it now...

How did you test it btw?

Currently the target driver doesn't support ipv6 addresses.

These complementary patches [1,2] makes the it work e2e!

Quoted 2 lines

How did you test it btw?

I have a userspace target :).

Which is also how I caught the uninitialized private data.

Quoted 3 lines

Quoted 1 line

How did you test it btw?

I have a userspace target :).

Currently the target driver doesn't support ipv6 addresses.

These complementary patches [1,2] makes the it work e2e! So,

Quoted 4 lines

Currently the target driver doesn't support ipv6 addresses.

These complementary patches [1,2] makes the it work e2e! So,

I think we should use the same ipv6 addr parsing that Roland used in the host, and preferably have another module export that as a helper.

Quoted 5 lines

Quoted 4 lines

Currently the target driver doesn't support ipv6 addresses.

These complementary patches [1,2] makes the it work e2e! So,

Quoted 2 lines

I think we should use the same ipv6 addr parsing that Roland used in the host, and preferably have another module export that as a helper.

I can try to unify them, but unlike the host that first tries ipv4 and then fall to ipv6, the target needs to enforce the address family so it's somewhat different. I kinda think that these are different enough to stay separate don't you think?

It'd still need all the scope ID handling similar to what Roland did, and that's a fair chunk of code. We have a few options to handle the different allowed addresses:

(1) v4/v6 only flags
(2) having low-level v4/v6 handlers and one that tries these both
(3) using the try both handler and rejecting the wrong one after parsing.

(3) seems easiest, but (2) sounds fine to me. But I'd really like to hear from folks on the netdev list what they think of that idea first.

Quoted 10 lines

It'd still need all the scope ID handling similar to what Roland did, and that's a fair chunk of code. We have a few options to handle the different allowed addresses:

(1) v4/v6 only flags
(2) having low-level v4/v6 handlers and one that tries these both
(3) using the try both handler and rejecting the wrong one after parsing.

Quoted 2 lines

(3) seems easiest, but (2) sounds fine to me. But I'd really like to hear from folks on the netdev list what they think of that idea first.

I think adding a new helper that parses both v4 and v6 addresses + scope ID seems like the best thing for now. I did a grep for in6_pton and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c could use the helper.

What do you think of adding inet_pton_with_scope() to net/core/utils.c? I'm open to better ideas on the name. But I can code that up and use it in nvme, as well as convert over the two places I mentioned above. The first parameter of the function can be an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to restrict the parsing to one type of address (or not).

Quoted 11 lines · Christoph Hellwig

It'd still need all the scope ID handling similar to what Roland did, and that's a fair chunk of code. We have a few options to handle the different allowed addresses:

(1) v4/v6 only flags
(2) having low-level v4/v6 handlers and one that tries these both
(3) using the try both handler and rejecting the wrong one after parsing.

(3) seems easiest, but (2) sounds fine to me. But I'd really like to hear from folks on the netdev list what they think of that idea first.

I think adding a new helper that parses both v4 and v6 addresses + scope ID seems like the best thing for now. I did a grep for in6_pton and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c could use the helper.

What do you think of adding inet_pton_with_scope() to net/core/utils.c? I'm open to better ideas on the name. But I can code that up and use it in nvme, as well as convert over the two places I mentioned above. The first parameter of the function can be an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to restrict the parsing to one type of address (or not).

I think adding a new helper that parses both v4 and v6 addresses + scope ID seems like the best thing for now. I did a grep for in6_pton and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c could use the helper.

What do you think of adding inet_pton_with_scope() to net/core/utils.c? I'm open to better ideas on the name. But I can code that up and use it in nvme, as well as convert over the two places I mentioned above. The first parameter of the function can be an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to restrict the parsing to one type of address (or not).

Are you looking into this?

I'd love to get it into 4.9...

Quoted 3 lines

Are you looking into this?

I'd love to get it into 4.9...

Yeah, me too. I got distracted about halfway through writing the patch but I will finish it this week.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Currently the target driver doesn't support ipv6 addresses.

These complementary patches [1,2] makes the it work e2e! So,

Quoted 7 lines

Quoted 2 lines

I think we should use the same ipv6 addr parsing that Roland used in the host, and preferably have another module export that as a helper.

I can try to unify them, but unlike the host that first tries ipv4 and then fall to ipv6, the target needs to enforce the address family so it's somewhat different. I kinda think that these are different enough to stay separate don't you think?

It'd still need all the scope ID handling similar to what Roland did, and that's a fair chunk of code. We have a few options to handle the different allowed addresses:

(1) v4/v6 only flags
(2) having low-level v4/v6 handlers and one that tries these both
(3) using the try both handler and rejecting the wrong one after parsing.

(3) seems easiest, but (2) sounds fine to me. But I'd really like to hear from folks on the netdev list what they think of that idea first.

From: Roland Dreier <roland at purestorage.com>

If a target address does not parse as IPv4, try parsing it as IPv6 (including handling '%<scope-id>' suffixes for link-local addresses).

Signed-off-by Roland Dreier <roland at purestorage.com>

---
 drivers/nvme/host/rdma.c | 55 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 11 deletions(-)

drivers/nvme/host/rdma.c +44 -11

index b96b88369871..dd4aa54cd709 100644
@@ -138,6 +138,7 @@ struct nvme_rdma_ctrl {
 	union {
 		struct sockaddr addr;
 		struct sockaddr_in addr_in;
+		struct sockaddr_in6 addr_in6;
 	};
 
 	struct nvme_ctrl	ctrl;

@@ -1847,19 +1848,51 @@ out_free_io_queues:
 	return ret;
 }
 
-static int nvme_rdma_parse_ipaddr(struct sockaddr_in *in_addr, char *p)
+static int nvme_rdma_parse_ipaddr(struct sockaddr *addr, char *p)
 {
-	u8 *addr = (u8 *)&in_addr->sin_addr.s_addr;
 	size_t buflen = strlen(p);
 
-	/* XXX: handle IPv6 addresses */
+	if (buflen <= INET_ADDRSTRLEN) {
+		struct sockaddr_in *addr4 = (struct sockaddr_in *) addr;
+		if (in4_pton(p, buflen, (u8 *) &addr4->sin_addr.s_addr,
+			     '\0', NULL) > 0) {
+			addr4->sin_family = AF_INET;
+			return 0;
+		}
+	}
 
-	if (buflen > INET_ADDRSTRLEN)
-		return -EINVAL;
-	if (in4_pton(p, buflen, addr, '\0', NULL) == 0)
-		return -EINVAL;
-	in_addr->sin_family = AF_INET;
-	return 0;
+	if (buflen <= INET6_ADDRSTRLEN) {
+		struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *) addr;
+		const char *scope_delim;
+
+		if (in6_pton(p, buflen, (u8 *) &addr6->sin6_addr.s6_addr,
+			     '%', &scope_delim) == 0)
+			return -EINVAL;
+		addr6->sin6_family = AF_INET6;
+
+		if (ipv6_addr_type(&addr6->sin6_addr) & IPV6_ADDR_LINKLOCAL &&
+		    p + buflen != scope_delim && *scope_delim == '%') {
+			char scope_id[16];
+			size_t scope_len = min_t(size_t, sizeof scope_id,
+						 p + buflen - scope_delim - 1);
+			struct net_device *dev;
+
+			memcpy(scope_id, scope_delim + 1, scope_len);
+			scope_id[scope_len] = '\0';
+
+			/* XXX: what network namespace should we use? */
+			dev = dev_get_by_name(&init_net, scope_id);
+			if (dev) {
+				addr6->sin6_scope_id = dev->ifindex;
+				dev_put(dev);
+			} else if (kstrtouint(scope_id, 0, &addr6->sin6_scope_id))
+				return -EINVAL;
+		}
+
+		return 0;
+	}
+
+	return -EINVAL;
 }
 
 static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,

@@ -1875,7 +1908,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	ctrl->ctrl.opts = opts;
 	INIT_LIST_HEAD(&ctrl->list);
 
-	ret = nvme_rdma_parse_ipaddr(&ctrl->addr_in, opts->traddr);
+	ret = nvme_rdma_parse_ipaddr(&ctrl->addr, opts->traddr);
 	if (ret) {
 		pr_err("malformed IP address passed: %s\n", opts->traddr);
 		goto out_free_ctrl;

@@ -1949,7 +1982,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
 	WARN_ON_ONCE(!changed);
 
-	dev_info(ctrl->ctrl.device, "new ctrl: NQN \"%s\", addr %pISp\n",
+	dev_info(ctrl->ctrl.device, "new ctrl: NQN \"%s\", addr %pISpcs\n",
 		ctrl->ctrl.opts->subsysnqn, &ctrl->addr);
 
 	kref_get(&ctrl->ctrl.kref);

Signature · 1 line

2.7.4

From: Roland Dreier <roland at purestorage.com>

Zero out the full nvme_rdma_cm_req structure before sending it. Otherwise we end up leaking kernel memory in the reserved field, which might break forward compatibility in the future.

Signed-off-by Roland Dreier <roland at purestorage.com>