[PATCH 0/3] xen-netback: hash mapping hanling adjustments

STALE2880d

17 messages, 4 authors, 2018-09-25 · open the first message on its own page

[PATCH 0/3] xen-netback: hash mapping hanling adjustments

From: Jan Beulich <hidden>
Date: 2018-08-28 14:54:27

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] xen-netback: fix input validation in xenvif_set_hash_mapping()

From: Jan Beulich <hidden>
Date: 2018-08-28 14:59:10

Both len and off are frontend specified values, so we need to make
sure there's no overflow when adding the two for the bounds check. We
also want to avoid undefined behavior and hence use off to index into
->hash.mapping[] only after bounds checking. This at the same time
allows to take care of not applying off twice for the bounds checking
against vif->num_queues.

It is also insufficient to bounds check copy_op.len, as this is len
truncated to 16 bits.

This is XSA-270 / CVE-2018-15471.

Reported-by: Felix Wilhelm <redacted>
Signed-off-by: Jan Beulich <redacted>
Reviewed-by: Paul Durrant <redacted>
Tested-by: Paul Durrant <redacted>
Cc: stable@vger.kernel.org [4.7 onwards]

---
 drivers/net/xen-netback/hash.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -332,20 +332,22 @@ u32 xenvif_set_hash_mapping_size(struct
 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 			    u32 off)
 {
-	u32 *mapping = &vif->hash.mapping[off];
+	u32 *mapping = vif->hash.mapping;
 	struct gnttab_copy copy_op = {
 		.source.u.ref = gref,
 		.source.domid = vif->domid,
-		.dest.u.gmfn = virt_to_gfn(mapping),
 		.dest.domid = DOMID_SELF,
-		.dest.offset = xen_offset_in_page(mapping),
-		.len = len * sizeof(u32),
+		.len = len * sizeof(*mapping),
 		.flags = GNTCOPY_source_gref
 	};
 
-	if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE)
+	if ((off + len < off) || (off + len > vif->hash.size) ||
+	    len > XEN_PAGE_SIZE / sizeof(*mapping))
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
+	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
+	copy_op.dest.offset = xen_offset_in_page(mapping + off);
+
 	while (len-- != 0)
 		if (mapping[off++] >= vif->num_queues)
 			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] xen-netback: validate queue numbers in xenvif_set_hash_mapping()

From: Jan Beulich <hidden>
Date: 2018-08-28 18:51:53

Checking them before the grant copy means nothing as to the validity of
the incoming request. As we shouldn't make the new data live before
having validated it, introduce a second instance of the mapping array.

Signed-off-by: Jan Beulich <redacted>

---
 drivers/net/xen-netback/common.h    |    3 ++-
 drivers/net/xen-netback/hash.c      |   20 ++++++++++++++------
 drivers/net/xen-netback/interface.c |    3 ++-
 3 files changed, 18 insertions(+), 8 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/common.h
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/common.h

@@ -241,8 +241,9 @@ struct xenvif_hash_cache {
 struct xenvif_hash {
 	unsigned int alg;
 	u32 flags;
+	bool mapping_sel;
 	u8 key[XEN_NETBK_MAX_HASH_KEY_SIZE];
-	u32 mapping[XEN_NETBK_MAX_HASH_MAPPING_SIZE];
+	u32 mapping[2][XEN_NETBK_MAX_HASH_MAPPING_SIZE];
 	unsigned int size;
 	struct xenvif_hash_cache cache;
 };
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -324,7 +324,8 @@ u32 xenvif_set_hash_mapping_size(struct
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
 	vif->hash.size = size;
-	memset(vif->hash.mapping, 0, sizeof(u32) * size);
+	memset(vif->hash.mapping[vif->hash.mapping_sel], 0,
+	       sizeof(u32) * size);
 
 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }

@@ -332,7 +333,7 @@ u32 xenvif_set_hash_mapping_size(struct
 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 			    u32 off)
 {
-	u32 *mapping = vif->hash.mapping;
+	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
 	struct gnttab_copy copy_op = {
 		.source.u.ref = gref,
 		.source.domid = vif->domid,

@@ -348,9 +349,8 @@ u32 xenvif_set_hash_mapping(struct xenvi
 	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
 	copy_op.dest.offset = xen_offset_in_page(mapping + off);
 
-	while (len-- != 0)
-		if (mapping[off++] >= vif->num_queues)
-			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
+	       vif->hash.size * sizeof(*mapping));
 
 	if (copy_op.len != 0) {
 		gnttab_batch_copy(&copy_op, 1);

@@ -359,6 +359,12 @@ u32 xenvif_set_hash_mapping(struct xenvi
 			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 	}
 
+	while (len-- != 0)
+		if (mapping[off++] >= vif->num_queues)
+			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+
+	vif->hash.mapping_sel = !vif->hash.mapping_sel;
+
 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }

@@ -410,6 +416,8 @@ void xenvif_dump_hash_info(struct xenvif
 	}
 
 	if (vif->hash.size != 0) {
+		const u32 *mapping = vif->hash.mapping[vif->hash.mapping_sel];
+
 		seq_puts(m, "\nHash Mapping:\n");
 
 		for (i = 0; i < vif->hash.size; ) {

@@ -422,7 +430,7 @@ void xenvif_dump_hash_info(struct xenvif
 			seq_printf(m, "[%4u - %4u]: ", i, i + n - 1);
 
 			for (j = 0; j < n; j++, i++)
-				seq_printf(m, "%4u ", vif->hash.mapping[i]);
+				seq_printf(m, "%4u ", mapping[i]);
 
 			seq_puts(m, "\n");
 		}
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/interface.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/interface.c

@@ -162,7 +162,8 @@ static u16 xenvif_select_queue(struct ne
 	if (size == 0)
 		return skb_get_hash_raw(skb) % dev->real_num_tx_queues;
 
-	return vif->hash.mapping[skb_get_hash_raw(skb) % size];
+	return vif->hash.mapping[vif->hash.mapping_sel]
+				[skb_get_hash_raw(skb) % size];
 }
 
 static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)

[PATCH 3/3] xen-netback: handle page straddling in xenvif_set_hash_mapping()

From: Jan Beulich <hidden>
Date: 2018-08-28 18:52:22

There's no guarantee that the mapping array doesn't cross a page
boundary. Use a second grant copy operation if necessary.

Signed-off-by: Jan Beulich <redacted>

---
 drivers/net/xen-netback/hash.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -334,28 +334,39 @@ u32 xenvif_set_hash_mapping(struct xenvi
 			    u32 off)
 {
 	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
-	struct gnttab_copy copy_op = {
+	unsigned int nr = 1;
+	struct gnttab_copy copy_op[2] = {{
 		.source.u.ref = gref,
 		.source.domid = vif->domid,
 		.dest.domid = DOMID_SELF,
 		.len = len * sizeof(*mapping),
 		.flags = GNTCOPY_source_gref
-	};
+	}};
 
 	if ((off + len < off) || (off + len > vif->hash.size) ||
 	    len > XEN_PAGE_SIZE / sizeof(*mapping))
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
-	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
-	copy_op.dest.offset = xen_offset_in_page(mapping + off);
+	copy_op[0].dest.u.gmfn = virt_to_gfn(mapping + off);
+	copy_op[0].dest.offset = xen_offset_in_page(mapping + off);
+	if (copy_op[0].dest.offset + copy_op[0].len > XEN_PAGE_SIZE) {
+		copy_op[1] = copy_op[0];
+		copy_op[1].source.offset = XEN_PAGE_SIZE - copy_op[0].dest.offset;
+		copy_op[1].dest.u.gmfn = virt_to_gfn(mapping + off + len);
+		copy_op[1].dest.offset = 0;
+		copy_op[1].len = copy_op[0].len - copy_op[1].source.offset;
+		copy_op[0].len = copy_op[1].source.offset;
+		nr = 2;
+	}
 
 	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
 	       vif->hash.size * sizeof(*mapping));
 
-	if (copy_op.len != 0) {
-		gnttab_batch_copy(&copy_op, 1);
+	if (copy_op[0].len != 0) {
+		gnttab_batch_copy(copy_op, nr);
 
-		if (copy_op.status != GNTST_okay)
+		if (copy_op[0].status != GNTST_okay ||
+		    copy_op[nr - 1].status != GNTST_okay)
 			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 	}

Re: [PATCH 2/3] xen-netback: validate queue numbers in xenvif_set_hash_mapping()

From: Wei Liu <hidden>
Date: 2018-08-29 12:20:55

On Tue, Aug 28, 2018 at 08:59:45AM -0600, Jan Beulich wrote:

quoted hunk

Checking them before the grant copy means nothing as to the validity of
the incoming request. As we shouldn't make the new data live before
having validated it, introduce a second instance of the mapping array.

Signed-off-by: Jan Beulich <redacted>

---
 drivers/net/xen-netback/common.h    |    3 ++-
 drivers/net/xen-netback/hash.c      |   20 ++++++++++++++------
 drivers/net/xen-netback/interface.c |    3 ++-
 3 files changed, 18 insertions(+), 8 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/common.h
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/common.h

@@ -241,8 +241,9 @@ struct xenvif_hash_cache {
 struct xenvif_hash {
 	unsigned int alg;
 	u32 flags;
+	bool mapping_sel;
 	u8 key[XEN_NETBK_MAX_HASH_KEY_SIZE];
-	u32 mapping[XEN_NETBK_MAX_HASH_MAPPING_SIZE];
+	u32 mapping[2][XEN_NETBK_MAX_HASH_MAPPING_SIZE];
 	unsigned int size;
 	struct xenvif_hash_cache cache;
 };
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -324,7 +324,8 @@ u32 xenvif_set_hash_mapping_size(struct
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
 	vif->hash.size = size;
-	memset(vif->hash.mapping, 0, sizeof(u32) * size);
+	memset(vif->hash.mapping[vif->hash.mapping_sel], 0,
+	       sizeof(u32) * size);
 
 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }

@@ -332,7 +333,7 @@ u32 xenvif_set_hash_mapping_size(struct
 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 			    u32 off)
 {
-	u32 *mapping = vif->hash.mapping;
+	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];

Can you rename this to inactive_mapping so the code can be followed more
easily?

The code looks correct to me, but I would like Paul to have a look
before it can go in.

Wei.

Re: [PATCH 3/3] xen-netback: handle page straddling in xenvif_set_hash_mapping()

From: Wei Liu <hidden>
Date: 2018-08-29 12:21:54

On Tue, Aug 28, 2018 at 09:00:14AM -0600, Jan Beulich wrote:

There's no guarantee that the mapping array doesn't cross a page
boundary. Use a second grant copy operation if necessary.

Signed-off-by: Jan Beulich <redacted>

Acked-by: Wei Liu <redacted>

RE: [PATCH 2/3] xen-netback: validate queue numbers in xenvif_set_hash_mapping()

From: Paul Durrant <hidden>
Date: 2018-09-03 13:43:12

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com]
Sent: 28 August 2018 16:00
To: Paul Durrant <redacted>; Wei Liu <redacted>
Cc: davem@davemloft.net; xen-devel <redacted>;
netdev@vger.kernel.org
Subject: [PATCH 2/3] xen-netback: validate queue numbers in
xenvif_set_hash_mapping()

Checking them before the grant copy means nothing as to the validity of
the incoming request. As we shouldn't make the new data live before
having validated it, introduce a second instance of the mapping array.

Signed-off-by: Jan Beulich <redacted>

Reviewed-by: Paul Durrant <redacted>

quoted hunk

---
 drivers/net/xen-netback/common.h    |    3 ++-
 drivers/net/xen-netback/hash.c      |   20 ++++++++++++++------
 drivers/net/xen-netback/interface.c |    3 ++-
 3 files changed, 18 insertions(+), 8 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-

netback/common.h

+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-

netback/common.h

@@ -241,8 +241,9 @@ struct xenvif_hash_cache {
 struct xenvif_hash {
 	unsigned int alg;
 	u32 flags;
+	bool mapping_sel;
 	u8 key[XEN_NETBK_MAX_HASH_KEY_SIZE];
-	u32 mapping[XEN_NETBK_MAX_HASH_MAPPING_SIZE];
+	u32 mapping[2][XEN_NETBK_MAX_HASH_MAPPING_SIZE];
 	unsigned int size;
 	struct xenvif_hash_cache cache;
 };
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-

netback/hash.c

+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-

netback/hash.c

@@ -324,7 +324,8 @@ u32 xenvif_set_hash_mapping_size(struct
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;

 	vif->hash.size = size;
-	memset(vif->hash.mapping, 0, sizeof(u32) * size);
+	memset(vif->hash.mapping[vif->hash.mapping_sel], 0,
+	       sizeof(u32) * size);

 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }
@@ -332,7 +333,7 @@ u32 xenvif_set_hash_mapping_size(struct
 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 			    u32 off)
 {
-	u32 *mapping = vif->hash.mapping;
+	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
 	struct gnttab_copy copy_op = {
 		.source.u.ref = gref,
 		.source.domid = vif->domid,
@@ -348,9 +349,8 @@ u32 xenvif_set_hash_mapping(struct xenvi
 	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
 	copy_op.dest.offset = xen_offset_in_page(mapping + off);

-	while (len-- != 0)
-		if (mapping[off++] >= vif->num_queues)
-			return

XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
+	       vif->hash.size * sizeof(*mapping));

 	if (copy_op.len != 0) {
 		gnttab_batch_copy(&copy_op, 1);

@@ -359,6 +359,12 @@ u32 xenvif_set_hash_mapping(struct xenvi
 			return

XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 	}

+	while (len-- != 0)
+		if (mapping[off++] >= vif->num_queues)
+			return
XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+
+	vif->hash.mapping_sel = !vif->hash.mapping_sel;
+
 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }

@@ -410,6 +416,8 @@ void xenvif_dump_hash_info(struct xenvif
 	}

 	if (vif->hash.size != 0) {
+		const u32 *mapping = vif->hash.mapping[vif-

quoted

hash.mapping_sel];

+
 		seq_puts(m, "\nHash Mapping:\n");

 		for (i = 0; i < vif->hash.size; ) {

@@ -422,7 +430,7 @@ void xenvif_dump_hash_info(struct xenvif
 			seq_printf(m, "[%4u - %4u]: ", i, i + n - 1);

 			for (j = 0; j < n; j++, i++)
-				seq_printf(m, "%4u ", vif->hash.mapping[i]);
+				seq_printf(m, "%4u ", mapping[i]);

 			seq_puts(m, "\n");
 		}
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-

netback/interface.c

+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-

netback/interface.c

@@ -162,7 +162,8 @@ static u16 xenvif_select_queue(struct ne
 	if (size == 0)
 		return skb_get_hash_raw(skb) % dev-

quoted

real_num_tx_queues;

-	return vif->hash.mapping[skb_get_hash_raw(skb) % size];
+	return vif->hash.mapping[vif->hash.mapping_sel]
+				[skb_get_hash_raw(skb) % size];
 }

 static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)

RE: [PATCH 3/3] xen-netback: handle page straddling in xenvif_set_hash_mapping()

From: Paul Durrant <hidden>
Date: 2018-09-03 13:48:05

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com]
Sent: 28 August 2018 16:00
To: Paul Durrant <redacted>; Wei Liu <redacted>
Cc: davem@davemloft.net; xen-devel <redacted>;
netdev@vger.kernel.org
Subject: [PATCH 3/3] xen-netback: handle page straddling in
xenvif_set_hash_mapping()

There's no guarantee that the mapping array doesn't cross a page
boundary. Use a second grant copy operation if necessary.

Signed-off-by: Jan Beulich <redacted>

Personally I think it would be cleaner to out-of-line the allocation of the mapping table and ensure it is page aligned but this works so...

Reviewed-by: Paul Durrant <redacted>

quoted hunk

---
 drivers/net/xen-netback/hash.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-

netback/hash.c

+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-

netback/hash.c

@@ -334,28 +334,39 @@ u32 xenvif_set_hash_mapping(struct xenvi
 			    u32 off)
 {
 	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
-	struct gnttab_copy copy_op = {
+	unsigned int nr = 1;
+	struct gnttab_copy copy_op[2] = {{
 		.source.u.ref = gref,
 		.source.domid = vif->domid,
 		.dest.domid = DOMID_SELF,
 		.len = len * sizeof(*mapping),
 		.flags = GNTCOPY_source_gref
-	};
+	}};

 	if ((off + len < off) || (off + len > vif->hash.size) ||
 	    len > XEN_PAGE_SIZE / sizeof(*mapping))
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;

-	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
-	copy_op.dest.offset = xen_offset_in_page(mapping + off);
+	copy_op[0].dest.u.gmfn = virt_to_gfn(mapping + off);
+	copy_op[0].dest.offset = xen_offset_in_page(mapping + off);
+	if (copy_op[0].dest.offset + copy_op[0].len > XEN_PAGE_SIZE) {
+		copy_op[1] = copy_op[0];
+		copy_op[1].source.offset = XEN_PAGE_SIZE -

copy_op[0].dest.offset;
+		copy_op[1].dest.u.gmfn = virt_to_gfn(mapping + off + len);
+		copy_op[1].dest.offset = 0;
+		copy_op[1].len = copy_op[0].len - copy_op[1].source.offset;
+		copy_op[0].len = copy_op[1].source.offset;
+		nr = 2;
+	}

 	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
 	       vif->hash.size * sizeof(*mapping));

-	if (copy_op.len != 0) {
-		gnttab_batch_copy(&copy_op, 1);
+	if (copy_op[0].len != 0) {
+		gnttab_batch_copy(copy_op, nr);

-		if (copy_op.status != GNTST_okay)
+		if (copy_op[0].status != GNTST_okay ||
+		    copy_op[nr - 1].status != GNTST_okay)
 			return
XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 	}

Re: [PATCH 0/3] xen-netback: hash mapping hanling adjustments

From: Jan Beulich <hidden>
Date: 2018-09-11 13:10:20

quoted

On 28.08.18 at 16:54,  wrote:

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>

What is the way forward here? I've got R-b-s from Paul for all three
patches, and a minor change request on patch 2 from Wei. I'm not
really certain what to do in this case (hints appreciated), but could
at least the security fix (patch 1) be applied immediately?

Jan

Re: [PATCH 0/3] xen-netback: hash mapping hanling adjustments

From: Wei Liu <hidden>
Date: 2018-09-11 15:15:03

On Tue, Sep 11, 2018 at 02:12:07AM -0600, Jan Beulich wrote:

quoted

On 28.08.18 at 16:54,  wrote:

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>

What is the way forward here? I've got R-b-s from Paul for all three
patches, and a minor change request on patch 2 from Wei. I'm not
really certain what to do in this case (hints appreciated), but could
at least the security fix (patch 1) be applied immediately?

If you happen to resend, please make the adjustment; otherwise I'm fine
with the patches as they are. I don't want to block useful things on
cosmetic issues.

Wei.

Ping: [PATCH 0/3] xen-netback: hash mapping hanling adjustments

From: Jan Beulich <hidden>
Date: 2018-09-24 13:44:40

quoted

On 11.09.18 at 12:16, [off-list ref] wrote:

On Tue, Sep 11, 2018 at 02:12:07AM -0600, Jan Beulich wrote:

quoted

On 28.08.18 at 16:54,  wrote:

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>

What is the way forward here? I've got R-b-s from Paul for all three
patches, and a minor change request on patch 2 from Wei. I'm not
really certain what to do in this case (hints appreciated), but could
at least the security fix (patch 1) be applied immediately?

If you happen to resend, please make the adjustment; otherwise I'm fine
with the patches as they are. I don't want to block useful things on
cosmetic issues.

Dave? I notice none of the patches is in 4.19-rc5, not even the security
fix, the advisory for which had gone public over a month ago.

Jan

Re: Ping: [PATCH 0/3] xen-netback: hash mapping hanling adjustments

From: David Miller <davem@davemloft.net>
Date: 2018-09-24 22:52:23

From: "Jan Beulich" <redacted>
Date: Mon, 24 Sep 2018 01:43:50 -0600

quoted

On 11.09.18 at 12:16, [off-list ref] wrote:

On Tue, Sep 11, 2018 at 02:12:07AM -0600, Jan Beulich wrote:

quoted

On 28.08.18 at 16:54,  wrote:

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>

What is the way forward here? I've got R-b-s from Paul for all three
patches, and a minor change request on patch 2 from Wei. I'm not
really certain what to do in this case (hints appreciated), but could
at least the security fix (patch 1) be applied immediately?

If you happen to resend, please make the adjustment; otherwise I'm fine
with the patches as they are. I don't want to block useful things on
cosmetic issues.

Dave? I notice none of the patches is in 4.19-rc5, not even the security
fix, the advisory for which had gone public over a month ago.

If it's not in my patchwork queue, you have to resend the series and
make it clear that it should be applied to the networking tree by
putting "[PATCH net N/M]" in the Subject lines.

Thank you.

[PATCH net 0/3 RESEND] xen-netback: hash mapping handling adjustments

From: Jan Beulich <hidden>
Date: 2018-09-25 14:17:59

(re-send just to satisfy the apparent need for "net" inside the square brackets)

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>

[PATCH net 1/3 RESEND] xen-netback: fix input validation in xenvif_set_hash_mapping()

From: Jan Beulich <hidden>
Date: 2018-09-25 14:18:56

Both len and off are frontend specified values, so we need to make
sure there's no overflow when adding the two for the bounds check. We
also want to avoid undefined behavior and hence use off to index into
->hash.mapping[] only after bounds checking. This at the same time
allows to take care of not applying off twice for the bounds checking
against vif->num_queues.

It is also insufficient to bounds check copy_op.len, as this is len
truncated to 16 bits.

This is XSA-270 / CVE-2018-15471.

Reported-by: Felix Wilhelm <redacted>
Signed-off-by: Jan Beulich <redacted>
Reviewed-by: Paul Durrant <redacted>
Tested-by: Paul Durrant <redacted>
Cc: stable@vger.kernel.org [4.7 onwards]
---
 drivers/net/xen-netback/hash.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -332,20 +332,22 @@ u32 xenvif_set_hash_mapping_size(struct
 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 			    u32 off)
 {
-	u32 *mapping = &vif->hash.mapping[off];
+	u32 *mapping = vif->hash.mapping;
 	struct gnttab_copy copy_op = {
 		.source.u.ref = gref,
 		.source.domid = vif->domid,
-		.dest.u.gmfn = virt_to_gfn(mapping),
 		.dest.domid = DOMID_SELF,
-		.dest.offset = xen_offset_in_page(mapping),
-		.len = len * sizeof(u32),
+		.len = len * sizeof(*mapping),
 		.flags = GNTCOPY_source_gref
 	};
 
-	if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE)
+	if ((off + len < off) || (off + len > vif->hash.size) ||
+	    len > XEN_PAGE_SIZE / sizeof(*mapping))
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
+	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
+	copy_op.dest.offset = xen_offset_in_page(mapping + off);
+
 	while (len-- != 0)
 		if (mapping[off++] >= vif->num_queues)
 			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;

[PATCH net 2/3 RESEND] xen-netback: validate queue numbers in xenvif_set_hash_mapping()

From: Jan Beulich <hidden>
Date: 2018-09-25 14:19:25

Checking them before the grant copy means nothing as to the validity of
the incoming request. As we shouldn't make the new data live before
having validated it, introduce a second instance of the mapping array.

Signed-off-by: Jan Beulich <redacted>
Reviewed-by: Paul Durrant <redacted>
---
 drivers/net/xen-netback/common.h    |    3 ++-
 drivers/net/xen-netback/hash.c      |   20 ++++++++++++++------
 drivers/net/xen-netback/interface.c |    3 ++-
 3 files changed, 18 insertions(+), 8 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/common.h
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/common.h

@@ -241,8 +241,9 @@ struct xenvif_hash_cache {
 struct xenvif_hash {
 	unsigned int alg;
 	u32 flags;
+	bool mapping_sel;
 	u8 key[XEN_NETBK_MAX_HASH_KEY_SIZE];
-	u32 mapping[XEN_NETBK_MAX_HASH_MAPPING_SIZE];
+	u32 mapping[2][XEN_NETBK_MAX_HASH_MAPPING_SIZE];
 	unsigned int size;
 	struct xenvif_hash_cache cache;
 };
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -324,7 +324,8 @@ u32 xenvif_set_hash_mapping_size(struct
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
 	vif->hash.size = size;
-	memset(vif->hash.mapping, 0, sizeof(u32) * size);
+	memset(vif->hash.mapping[vif->hash.mapping_sel], 0,
+	       sizeof(u32) * size);
 
 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }

@@ -332,7 +333,7 @@ u32 xenvif_set_hash_mapping_size(struct
 u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 			    u32 off)
 {
-	u32 *mapping = vif->hash.mapping;
+	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
 	struct gnttab_copy copy_op = {
 		.source.u.ref = gref,
 		.source.domid = vif->domid,

@@ -348,9 +349,8 @@ u32 xenvif_set_hash_mapping(struct xenvi
 	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
 	copy_op.dest.offset = xen_offset_in_page(mapping + off);
 
-	while (len-- != 0)
-		if (mapping[off++] >= vif->num_queues)
-			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
+	       vif->hash.size * sizeof(*mapping));
 
 	if (copy_op.len != 0) {
 		gnttab_batch_copy(&copy_op, 1);

@@ -359,6 +359,12 @@ u32 xenvif_set_hash_mapping(struct xenvi
 			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 	}
 
+	while (len-- != 0)
+		if (mapping[off++] >= vif->num_queues)
+			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+
+	vif->hash.mapping_sel = !vif->hash.mapping_sel;
+
 	return XEN_NETIF_CTRL_STATUS_SUCCESS;
 }

@@ -410,6 +416,8 @@ void xenvif_dump_hash_info(struct xenvif
 	}
 
 	if (vif->hash.size != 0) {
+		const u32 *mapping = vif->hash.mapping[vif->hash.mapping_sel];
+
 		seq_puts(m, "\nHash Mapping:\n");
 
 		for (i = 0; i < vif->hash.size; ) {

@@ -422,7 +430,7 @@ void xenvif_dump_hash_info(struct xenvif
 			seq_printf(m, "[%4u - %4u]: ", i, i + n - 1);
 
 			for (j = 0; j < n; j++, i++)
-				seq_printf(m, "%4u ", vif->hash.mapping[i]);
+				seq_printf(m, "%4u ", mapping[i]);
 
 			seq_puts(m, "\n");
 		}
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/interface.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/interface.c

@@ -162,7 +162,8 @@ static u16 xenvif_select_queue(struct ne
 	if (size == 0)
 		return skb_get_hash_raw(skb) % dev->real_num_tx_queues;
 
-	return vif->hash.mapping[skb_get_hash_raw(skb) % size];
+	return vif->hash.mapping[vif->hash.mapping_sel]
+				[skb_get_hash_raw(skb) % size];
 }
 
 static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)

[PATCH net 3/3 RESEND] xen-netback: handle page straddling in xenvif_set_hash_mapping()

From: Jan Beulich <hidden>
Date: 2018-09-25 14:20:03

There's no guarantee that the mapping array doesn't cross a page
boundary. Use a second grant copy operation if necessary.

Signed-off-by: Jan Beulich <redacted>
Acked-by: Wei Liu <redacted>
Reviewed-by: Paul Durrant <redacted>
---
 drivers/net/xen-netback/hash.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c

@@ -334,28 +334,39 @@ u32 xenvif_set_hash_mapping(struct xenvi
 			    u32 off)
 {
 	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
-	struct gnttab_copy copy_op = {
+	unsigned int nr = 1;
+	struct gnttab_copy copy_op[2] = {{
 		.source.u.ref = gref,
 		.source.domid = vif->domid,
 		.dest.domid = DOMID_SELF,
 		.len = len * sizeof(*mapping),
 		.flags = GNTCOPY_source_gref
-	};
+	}};
 
 	if ((off + len < off) || (off + len > vif->hash.size) ||
 	    len > XEN_PAGE_SIZE / sizeof(*mapping))
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
-	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
-	copy_op.dest.offset = xen_offset_in_page(mapping + off);
+	copy_op[0].dest.u.gmfn = virt_to_gfn(mapping + off);
+	copy_op[0].dest.offset = xen_offset_in_page(mapping + off);
+	if (copy_op[0].dest.offset + copy_op[0].len > XEN_PAGE_SIZE) {
+		copy_op[1] = copy_op[0];
+		copy_op[1].source.offset = XEN_PAGE_SIZE - copy_op[0].dest.offset;
+		copy_op[1].dest.u.gmfn = virt_to_gfn(mapping + off + len);
+		copy_op[1].dest.offset = 0;
+		copy_op[1].len = copy_op[0].len - copy_op[1].source.offset;
+		copy_op[0].len = copy_op[1].source.offset;
+		nr = 2;
+	}
 
 	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
 	       vif->hash.size * sizeof(*mapping));
 
-	if (copy_op.len != 0) {
-		gnttab_batch_copy(&copy_op, 1);
+	if (copy_op[0].len != 0) {
+		gnttab_batch_copy(copy_op, nr);
 
-		if (copy_op.status != GNTST_okay)
+		if (copy_op[0].status != GNTST_okay ||
+		    copy_op[nr - 1].status != GNTST_okay)
 			return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 	}

Re: [PATCH net 0/3 RESEND] xen-netback: hash mapping handling adjustments

From: David Miller <davem@davemloft.net>
Date: 2018-09-25 23:49:23

From: "Jan Beulich" <redacted>
Date: Tue, 25 Sep 2018 02:11:33 -0600

First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.

1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()

Signed-off-by: Jan Beulich <redacted>

Series applied and queued up for -stable.

Thanks.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help