Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net,v2 0/2] Fix send indirection table offset
@ 2019-11-21 21:33 Haiyang Zhang
  2019-11-21 21:33 ` [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table() Haiyang Zhang
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Haiyang Zhang @ 2019-11-21 21:33 UTC (permalink / raw)
  To: sashal, linux-hyperv, netdev
  Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel

Fix send indirection table offset issues related to guest and
host bugs.

Haiyang Zhang (2):
  hv_netvsc: Fix offset usage in netvsc_send_table()
  hv_netvsc: Fix send_table offset in case of a host bug

 drivers/net/hyperv/hyperv_net.h |  3 ++-
 drivers/net/hyperv/netvsc.c     | 38 ++++++++++++++++++++++++++++++--------
 2 files changed, 32 insertions(+), 9 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
  2019-11-21 21:33 [PATCH net,v2 0/2] Fix send indirection table offset Haiyang Zhang
@ 2019-11-21 21:33 ` Haiyang Zhang
  2019-11-21 23:04   ` Jakub Kicinski
  2019-11-21 21:33 ` [PATCH net,v2 2/2] hv_netvsc: Fix send_table offset in case of a host bug Haiyang Zhang
  2019-11-22  8:04 ` [PATCH net,v2 0/2] Fix send indirection table offset David Miller
  2 siblings, 1 reply; 7+ messages in thread
From: Haiyang Zhang @ 2019-11-21 21:33 UTC (permalink / raw)
  To: sashal, linux-hyperv, netdev
  Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel

To reach the data region, the existing code adds offset in struct
nvsp_5_send_indirect_table on the beginning of this struct. But the
offset should be based on the beginning of its container,
struct nvsp_message. This bug causes the first table entry missing,
and adds an extra zero from the zero pad after the data region.
This can put extra burden on the channel 0.

So, correct the offset usage. Also add a boundary check to ensure
not reading beyond data region.

Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling (vRSS)")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/hyperv_net.h |  3 ++-
 drivers/net/hyperv/netvsc.c     | 26 ++++++++++++++++++--------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 670ef68..fb547f3 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -609,7 +609,8 @@ struct nvsp_5_send_indirect_table {
 	/* The number of entries in the send indirection table */
 	u32 count;
 
-	/* The offset of the send indirection table from top of this struct.
+	/* The offset of the send indirection table from the beginning of
+	 * struct nvsp_message.
 	 * The send indirection table tells which channel to put the send
 	 * traffic on. Each entry is a channel number.
 	 */
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index d22a36f..9b0532e 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1178,20 +1178,28 @@ static int netvsc_receive(struct net_device *ndev,
 }
 
 static void netvsc_send_table(struct net_device *ndev,
-			      const struct nvsp_message *nvmsg)
+			      const struct nvsp_message *nvmsg,
+			      u32 msglen)
 {
 	struct net_device_context *net_device_ctx = netdev_priv(ndev);
-	u32 count, *tab;
+	u32 count, offset, *tab;
 	int i;
 
 	count = nvmsg->msg.v5_msg.send_table.count;
+	offset = nvmsg->msg.v5_msg.send_table.offset;
+
 	if (count != VRSS_SEND_TAB_SIZE) {
 		netdev_err(ndev, "Received wrong send-table size:%u\n", count);
 		return;
 	}
 
-	tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
-		      nvmsg->msg.v5_msg.send_table.offset);
+	if (offset > msglen - count * sizeof(u32)) {
+		netdev_err(ndev, "Received send-table offset too big:%u\n",
+			   offset);
+		return;
+	}
+
+	tab = (void *)nvmsg + offset;
 
 	for (i = 0; i < count; i++)
 		net_device_ctx->tx_table[i] = tab[i];
@@ -1209,12 +1217,13 @@ static void netvsc_send_vf(struct net_device *ndev,
 		    net_device_ctx->vf_alloc ? "added" : "removed");
 }
 
-static  void netvsc_receive_inband(struct net_device *ndev,
-				   const struct nvsp_message *nvmsg)
+static void netvsc_receive_inband(struct net_device *ndev,
+				  const struct nvsp_message *nvmsg,
+				  u32 msglen)
 {
 	switch (nvmsg->hdr.msg_type) {
 	case NVSP_MSG5_TYPE_SEND_INDIRECTION_TABLE:
-		netvsc_send_table(ndev, nvmsg);
+		netvsc_send_table(ndev, nvmsg, msglen);
 		break;
 
 	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
@@ -1232,6 +1241,7 @@ static int netvsc_process_raw_pkt(struct hv_device *device,
 {
 	struct vmbus_channel *channel = nvchan->channel;
 	const struct nvsp_message *nvmsg = hv_pkt_data(desc);
+	u32 msglen = hv_pkt_datalen(desc);
 
 	trace_nvsp_recv(ndev, channel, nvmsg);
 
@@ -1247,7 +1257,7 @@ static int netvsc_process_raw_pkt(struct hv_device *device,
 		break;
 
 	case VM_PKT_DATA_INBAND:
-		netvsc_receive_inband(ndev, nvmsg);
+		netvsc_receive_inband(ndev, nvmsg, msglen);
 		break;
 
 	default:
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net,v2 2/2] hv_netvsc: Fix send_table offset in case of a host bug
  2019-11-21 21:33 [PATCH net,v2 0/2] Fix send indirection table offset Haiyang Zhang
  2019-11-21 21:33 ` [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table() Haiyang Zhang
@ 2019-11-21 21:33 ` Haiyang Zhang
  2019-11-22  8:04 ` [PATCH net,v2 0/2] Fix send indirection table offset David Miller
  2 siblings, 0 replies; 7+ messages in thread
From: Haiyang Zhang @ 2019-11-21 21:33 UTC (permalink / raw)
  To: sashal, linux-hyperv, netdev
  Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel

If negotiated NVSP version <= NVSP_PROTOCOL_VERSION_6, the offset may
be wrong (too small) due to a host bug. This can cause missing the
end of the send indirection table, and add multiple zero entries from
leading zeros before the data region. This bug adds extra burden on
channel 0.

So fix the offset by computing it from the data structure sizes. This
will ensure netvsc driver runs normally on unfixed hosts, and future
fixed hosts.

Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling (vRSS)")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 9b0532e..eab83e7 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1178,6 +1178,7 @@ static int netvsc_receive(struct net_device *ndev,
 }
 
 static void netvsc_send_table(struct net_device *ndev,
+			      struct netvsc_device *nvscdev,
 			      const struct nvsp_message *nvmsg,
 			      u32 msglen)
 {
@@ -1193,6 +1194,16 @@ static void netvsc_send_table(struct net_device *ndev,
 		return;
 	}
 
+	/* If negotiated version <= NVSP_PROTOCOL_VERSION_6, the offset may be
+	 * wrong due to a host bug. So fix the offset here.
+	 */
+	if (nvscdev->nvsp_version <= NVSP_PROTOCOL_VERSION_6 &&
+	    msglen >= sizeof(struct nvsp_message_header) +
+	    sizeof(union nvsp_6_message_uber) + count * sizeof(u32))
+		offset = sizeof(struct nvsp_message_header) +
+			 sizeof(union nvsp_6_message_uber);
+
+	/* Boundary check for all versions */
 	if (offset > msglen - count * sizeof(u32)) {
 		netdev_err(ndev, "Received send-table offset too big:%u\n",
 			   offset);
@@ -1218,12 +1229,13 @@ static void netvsc_send_vf(struct net_device *ndev,
 }
 
 static void netvsc_receive_inband(struct net_device *ndev,
+				  struct netvsc_device *nvscdev,
 				  const struct nvsp_message *nvmsg,
 				  u32 msglen)
 {
 	switch (nvmsg->hdr.msg_type) {
 	case NVSP_MSG5_TYPE_SEND_INDIRECTION_TABLE:
-		netvsc_send_table(ndev, nvmsg, msglen);
+		netvsc_send_table(ndev, nvscdev, nvmsg, msglen);
 		break;
 
 	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
@@ -1257,7 +1269,7 @@ static int netvsc_process_raw_pkt(struct hv_device *device,
 		break;
 
 	case VM_PKT_DATA_INBAND:
-		netvsc_receive_inband(ndev, nvmsg, msglen);
+		netvsc_receive_inband(ndev, net_device, nvmsg, msglen);
 		break;
 
 	default:
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
  2019-11-21 21:33 ` [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table() Haiyang Zhang
@ 2019-11-21 23:04   ` Jakub Kicinski
  2019-11-22  0:54     ` Haiyang Zhang
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2019-11-21 23:04 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
	davem, linux-kernel

On Thu, 21 Nov 2019 13:33:40 -0800, Haiyang Zhang wrote:
> To reach the data region, the existing code adds offset in struct
> nvsp_5_send_indirect_table on the beginning of this struct. But the
> offset should be based on the beginning of its container,
> struct nvsp_message. This bug causes the first table entry missing,
> and adds an extra zero from the zero pad after the data region.
> This can put extra burden on the channel 0.
> 
> So, correct the offset usage. Also add a boundary check to ensure
> not reading beyond data region.

Please provide a change log at the end of the commit message when
posting new version in the future.

> Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling (vRSS)")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

>  
> -	tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
> -		      nvmsg->msg.v5_msg.send_table.offset);
> +	if (offset > msglen - count * sizeof(u32)) {

Can't this underflow now? What if msglen is small?

> +		netdev_err(ndev, "Received send-table offset too big:%u\n",
> +			   offset);
> +		return;
> +	}
> +
> +	tab = (void *)nvmsg + offset;
>  
>  	for (i = 0; i < count; i++)
>  		net_device_ctx->tx_table[i] = tab[i];

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
  2019-11-21 23:04   ` Jakub Kicinski
@ 2019-11-22  0:54     ` Haiyang Zhang
  2019-11-22  1:03       ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Haiyang Zhang @ 2019-11-22  0:54 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: sashal, linux-hyperv, netdev, KY Srinivasan, Stephen Hemminger,
	olaf, vkuznets, davem, linux-kernel



> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Thursday, November 21, 2019 6:05 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in
> netvsc_send_table()
> 
> On Thu, 21 Nov 2019 13:33:40 -0800, Haiyang Zhang wrote:
> > To reach the data region, the existing code adds offset in struct
> > nvsp_5_send_indirect_table on the beginning of this struct. But the
> > offset should be based on the beginning of its container,
> > struct nvsp_message. This bug causes the first table entry missing,
> > and adds an extra zero from the zero pad after the data region.
> > This can put extra burden on the channel 0.
> >
> > So, correct the offset usage. Also add a boundary check to ensure
> > not reading beyond data region.
> 
> Please provide a change log at the end of the commit message when
> posting new version in the future.
Sure. Will do that in the future.

> 
> > Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling
> (vRSS)")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> >
> > -	tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
> > -		      nvmsg->msg.v5_msg.send_table.offset);
> > +	if (offset > msglen - count * sizeof(u32)) {
> 
> Can't this underflow now? What if msglen is small?
msglen came from the vmbus container message. We trust it to be big
enough for the data region.

Thanks,
- Haiyang


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
  2019-11-22  0:54     ` Haiyang Zhang
@ 2019-11-22  1:03       ` Jakub Kicinski
  0 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2019-11-22  1:03 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: sashal, linux-hyperv, netdev, KY Srinivasan, Stephen Hemminger,
	olaf, vkuznets, davem, linux-kernel

On Fri, 22 Nov 2019 00:54:20 +0000, Haiyang Zhang wrote:
> > >
> > > -	tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
> > > -		      nvmsg->msg.v5_msg.send_table.offset);
> > > +	if (offset > msglen - count * sizeof(u32)) {  
> > 
> > Can't this underflow now? What if msglen is small?  
> msglen came from the vmbus container message. We trust it to be big
> enough for the data region.

Ok, it looked like it was read from some descriptor which could
potentially be controlled by "the other side" but I trust your
judgement :)

Both patches LGTM, then.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net,v2 0/2] Fix send indirection table offset
  2019-11-21 21:33 [PATCH net,v2 0/2] Fix send indirection table offset Haiyang Zhang
  2019-11-21 21:33 ` [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table() Haiyang Zhang
  2019-11-21 21:33 ` [PATCH net,v2 2/2] hv_netvsc: Fix send_table offset in case of a host bug Haiyang Zhang
@ 2019-11-22  8:04 ` David Miller
  2 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2019-11-22  8:04 UTC (permalink / raw)
  To: haiyangz
  Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
	linux-kernel

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Thu, 21 Nov 2019 13:33:39 -0800

> Fix send indirection table offset issues related to guest and
> host bugs.

Series applied, thank you.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-21 21:33 [PATCH net,v2 0/2] Fix send indirection table offset Haiyang Zhang
2019-11-21 21:33 ` [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table() Haiyang Zhang
2019-11-21 23:04   ` Jakub Kicinski
2019-11-22  0:54     ` Haiyang Zhang
2019-11-22  1:03       ` Jakub Kicinski
2019-11-21 21:33 ` [PATCH net,v2 2/2] hv_netvsc: Fix send_table offset in case of a host bug Haiyang Zhang
2019-11-22  8:04 ` [PATCH net,v2 0/2] Fix send indirection table offset David Miller

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git