-----Original Message-----
From: Michael Kelley <redacted>
Sent: Monday, November 18, 2019 3:47 PM
To: Haiyang Zhang <haiyangz@microsoft.com>; vkuznets
[off-list ref]
Cc: KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
[off-list ref]; olaf@aepfle.de; davem@davemloft.net; linux-
kernel@vger.kernel.org; sashal@kernel.org; linux-hyperv@vger.kernel.org;
netdev@vger.kernel.org
Subject: RE: [PATCH net, 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()

From: Haiyang Zhang <haiyangz@microsoft.com> Sent: Monday, November 18,
2019 10:40 AM

quoted

quoted

-----Original Message-----
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Sent: Monday, November 18, 2019 12:29 PM

Haiyang Zhang [off-list ref] writes:

quoted

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

quoted

quoted

(vRSS)")

quoted

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

quoted

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..efd30e2 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,

quoted

quoted

quoted

 }

 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);

quoted

 		return;
 	}

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

Nit: I think this can overflow.

To prevent overflow, I will change it to:
	if (offset > msglen || offset + count * sizeof(u32) > msglen) {
Thanks,
- Haiyang

Actually, this would be simpler since we already trust msglen and count
to have good values:

	if (offset > msglen - count * sizeof(u32)) {

Great idea. Thanks!