hv_netvsc: Fix/improve RX path error handling

PATCH In review Haiyang Zhang 6 participants 10 messages Mar 22, 2018 – Mar 27, 2018

Subsystems (3)

Networking Network drivers Hyper-V/Azure core and drivers

lore

Share & tools

lore

Haiyang Zhang

author

2018-03-22 19:01 UTC #0 lore raw

cover hv_netvsc: Fix/improve RX path error handling

From: Haiyang Zhang <[email protected]>

Fix the status code returned to the host. Also add range check for rx packet offset and length.

Haiyang Zhang (2):
hv_netvsc: Fix the return status in RX path
hv_netvsc: Add range checking for rx packet offset and length

 drivers/net/hyperv/hyperv_net.h   |  1 +
 drivers/net/hyperv/netvsc.c       | 25 +++++++++++++++++++++----
 drivers/net/hyperv/netvsc_drv.c   |  2 +-
 drivers/net/hyperv/rndis_filter.c |  4 ++--
 4 files changed, 25 insertions(+), 7 deletions(-)

Signature · 6 lines

2.15.1

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Haiyang Zhang

author

2018-03-22 19:01 UTC #1 lore raw

PATCH 1/2 hv_netvsc: Fix the return status in RX path

From: Haiyang Zhang <[email protected]>

As defined in hyperv_net.h, the NVSP_STAT_SUCCESS is one not zero. Some functions returns 0 when it actually means NVSP_STAT_SUCCESS. This patch fixes them.

In netvsc_receive(), it puts the last RNDIS packet's receive status for all packets in a vmxferpage which may contain multiple RNDIS packets.
This patch puts NVSP_STAT_FAIL in the receive completion if one of the packets in a vmxferpage fails.

Signed-off-by Haiyang Zhang <[email protected]>

---
 drivers/net/hyperv/netvsc.c       | 8 ++++++--
 drivers/net/hyperv/netvsc_drv.c   | 2 +-
 drivers/net/hyperv/rndis_filter.c | 4 ++--
 3 files changed, 9 insertions(+), 5 deletions(-)

drivers/net/hyperv/netvsc.c +6 -2

index aa95e81af6e5..1ddb2c39b6e4 100644
@@ -1098,12 +1098,16 @@ static int netvsc_receive(struct net_device *ndev,
 		void *data = recv_buf
 			+ vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		int ret;
 
 		trace_rndis_recv(ndev, q_idx, data);
 
 		/* Pass it to the upper layer */
-		status = rndis_filter_receive(ndev, net_device,
-					      channel, data, buflen);
+		ret = rndis_filter_receive(ndev, net_device,
+					   channel, data, buflen);
+
+		if (unlikely(ret != NVSP_STAT_SUCCESS))
+			status = NVSP_STAT_FAIL;
 	}
 
 	enq_receive_complete(ndev, net_device, q_idx,

drivers/net/hyperv/netvsc_drv.c +1 -1

index cdb78eefab67..33607995be62 100644
@@ -818,7 +818,7 @@ int netvsc_recv_callback(struct net_device *net,
 	u64_stats_update_end(&rx_stats->syncp);
 
 	napi_gro_receive(&nvchan->napi, skb);
-	return 0;
+	return NVSP_STAT_SUCCESS;
 }
 
 static void netvsc_get_drvinfo(struct net_device *net,

drivers/net/hyperv/rndis_filter.c +2 -2

index 2dc00f714482..591fb8080f11 100644
@@ -443,10 +443,10 @@ int rndis_filter_receive(struct net_device *ndev,
 			"unhandled rndis message (type %u len %u)\n",
 			   rndis_msg->ndis_msg_type,
 			   rndis_msg->msg_len);
-		break;
+		return NVSP_STAT_FAIL;
 	}
 
-	return 0;
+	return NVSP_STAT_SUCCESS;
 }
 
 static int rndis_filter_query_device(struct rndis_device *dev,

Signature · 6 lines

2.15.1

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Michael Kelley (EOSG)

2018-03-24 16:48 UTC #2 lore raw

RE: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path

Quoted 16 lines

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Haiyang Zhang
Sent: Thursday, March 22, 2018 12:01 PM
To: [email protected]; [email protected]
Cc: Haiyang Zhang <[email protected]>; KY Srinivasan <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path
From: Haiyang Zhang <[email protected]>
As defined in hyperv_net.h, the NVSP_STAT_SUCCESS is one not zero. Some functions returns 0 when it actually means NVSP_STAT_SUCCESS. This patch fixes them.

Quoted 5 lines

In netvsc_receive(), it puts the last RNDIS packet's receive status for all packets in a vmxferpage which may contain multiple RNDIS packets.
This patch puts NVSP_STAT_FAIL in the receive completion if one of the packets in a vmxferpage fails.

This patch changes the status field that is being reported back to the Hyper-V host in the receive completion message in
enq_receive_complete(). The current code reports 0 on success,
and with the patch, it will report 1 on success. So does this change affect anything on the Hyper-V side? Or is Hyper-V just ignoring
the value? If this change doesn't have any impact on the
interactions with Hyper-V, perhaps it would be good to explain why in the commit message.

Michael

Quoted 62 lines

Signed-off-by Haiyang Zhang <[email protected]>

---
 drivers/net/hyperv/netvsc.c       | 8 ++++++--
 drivers/net/hyperv/netvsc_drv.c   | 2 +-
 drivers/net/hyperv/rndis_filter.c | 4 ++--
 3 files changed, 9 insertions(+), 5 deletions(-)

drivers/net/hyperv/netvsc.c +6 -2

index aa95e81af6e5..1ddb2c39b6e4 100644
@@ -1098,12 +1098,16 @@ static int netvsc_receive(struct net_device *ndev,
 		void *data = recv_buf
 			+ vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		int ret;
 
 		trace_rndis_recv(ndev, q_idx, data);
 
 		/* Pass it to the upper layer */
-		status = rndis_filter_receive(ndev, net_device,
-					      channel, data, buflen);
+		ret = rndis_filter_receive(ndev, net_device,
+					   channel, data, buflen);
+
+		if (unlikely(ret != NVSP_STAT_SUCCESS))
+			status = NVSP_STAT_FAIL;
 	}
 
 	enq_receive_complete(ndev, net_device, q_idx,

drivers/net/hyperv/netvsc_drv.c +1 -1

index cdb78eefab67..33607995be62 100644
@@ -818,7 +818,7 @@ int netvsc_recv_callback(struct net_device *net,
 	u64_stats_update_end(&rx_stats->syncp);
 
 	napi_gro_receive(&nvchan->napi, skb);
-	return 0;
+	return NVSP_STAT_SUCCESS;
 }
 
 static void netvsc_get_drvinfo(struct net_device *net,

drivers/net/hyperv/rndis_filter.c +2 -3

index 2dc00f714482..591fb8080f11 100644
@@ -443,10 +443,10 @@ int rndis_filter_receive(struct net_device *ndev,
 			"unhandled rndis message (type %u len %u)\n",
 			   rndis_msg->ndis_msg_type,
 			   rndis_msg->msg_len);
-		break;
+		return NVSP_STAT_FAIL;
 	}
 
-	return 0;
+	return NVSP_STAT_SUCCESS;
 }
 
 static int rndis_filter_query_device(struct rndis_device *dev,
--

Quoted 1 line

2.15.1

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Haiyang Zhang

maintainer

2018-03-25 00:41 UTC #3 lore raw

RE: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path

Quoted 36 lines

-----Original Message-----
From: Michael Kelley (EOSG)
Sent: Saturday, March 24, 2018 12:48 PM
To: Haiyang Zhang <[email protected]>; [email protected]; [email protected]
Cc: KY Srinivasan <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path
Quoted 18 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Haiyang Zhang
Sent: Thursday, March 22, 2018 12:01 PM
To: [email protected]; [email protected]
Cc: Haiyang Zhang <[email protected]>; KY Srinivasan <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH net-next,1/2] hv_netvsc: Fix the return status in RX path
From: Haiyang Zhang <[email protected]>
As defined in hyperv_net.h, the NVSP_STAT_SUCCESS is one not zero. Some functions returns 0 when it actually means NVSP_STAT_SUCCESS. This patch fixes them.
Quoted 5 lines
In netvsc_receive(), it puts the last RNDIS packet's receive status for all packets in a vmxferpage which may contain multiple RNDIS packets.
This patch puts NVSP_STAT_FAIL in the receive completion if one of the packets in a vmxferpage fails.

Quoted 8 lines

This patch changes the status field that is being reported back to the Hyper-V host in the receive completion message in
enq_receive_complete(). The current code reports 0 on success,
and with the patch, it will report 1 on success. So does this change affect anything on the Hyper-V side? Or is Hyper-V just ignoring
the value? If this change doesn't have any impact on the
interactions with Hyper-V, perhaps it would be good to explain why in the commit message.

Here is the definition of each status code for NetVSP.

enum {
        NVSP_STAT_NONE = 0,
        NVSP_STAT_SUCCESS,
        NVSP_STAT_FAIL,
        NVSP_STAT_PROTOCOL_TOO_NEW,
        NVSP_STAT_PROTOCOL_TOO_OLD,
        NVSP_STAT_INVALID_RNDIS_PKT,
        NVSP_STAT_BUSY,
        NVSP_STAT_PROTOCOL_UNSUPPORTED,
        NVSP_STAT_MAX,
};

Existing code returns NVSP_STAT_NONE = 0, and with this patch we return NVSP_STAT_SUCCESS = 1.
Based on testing, either way works for now. But for correctness and future stability (e.g. host side becomes more stringent), we should follow the protocol.

Thanks,
- Haiyang

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Haiyang Zhang

author

2018-03-22 19:01 UTC #4 lore raw

PATCH 2/2 hv_netvsc: Add range checking for rx packet offset and length

In reply to Haiyang Zhang

From: Haiyang Zhang <[email protected]>

This patch adds range checking for rx packet offset and length. It may only happen if there is a host side bug.

Signed-off-by Haiyang Zhang <[email protected]>

---
 drivers/net/hyperv/hyperv_net.h |  1 +
 drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

drivers/net/hyperv/hyperv_net.h +1 -0

index 0db3bd1ea06f..49c05ac894e5 100644
@@ -793,6 +793,7 @@ struct netvsc_device {
 
 	/* Receive buffer allocated by us but manages by NetVSP */
 	void *recv_buf;
+	u32 recv_buf_size; /* allocated bytes */
 	u32 recv_buf_gpadl_handle;
 	u32 recv_section_cnt;
 	u32 recv_section_size;

drivers/net/hyperv/netvsc.c +15 -2

index 1ddb2c39b6e4..a6700d65f206 100644
@@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
 		goto cleanup;
 	}
 
+	net_device->recv_buf_size = buf_size;
+
 	/*
 	 * Establish the gpadl handle for this buffer on this
 	 * channel.  Note: This call uses the vmbus connection rather

@@ -1095,11 +1097,22 @@ static int netvsc_receive(struct net_device *ndev,
 
 	/* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
 	for (i = 0; i < count; i++) {
-		void *data = recv_buf
-			+ vmxferpage_packet->ranges[i].byte_offset;
+		u32 offset = vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		void *data;
 		int ret;
 
+		if (unlikely(offset + buflen > net_device->recv_buf_size)) {
+			status = NVSP_STAT_FAIL;
+			netif_err(net_device_ctx, rx_err, ndev,
+				  "Packet offset:%u + len:%u too big\n",
+				  offset, buflen);
+
+			continue;
+		}
+
+		data = recv_buf + offset;
+
 		trace_rndis_recv(ndev, q_idx, data);
 
 		/* Pass it to the upper layer */

Signature · 6 lines

2.15.1

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Vitaly Kuznetsov

2018-03-23 15:17 UTC #5 lore raw

Re: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet offset and length

Quoted 42 lines · Haiyang Zhang

From: Haiyang Zhang <[email protected]>

This patch adds range checking for rx packet offset and length. It may only happen if there is a host side bug.

Signed-off-by Haiyang Zhang <[email protected]>

---
 drivers/net/hyperv/hyperv_net.h |  1 +
 drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

drivers/net/hyperv/hyperv_net.h +1 -0

index 0db3bd1ea06f..49c05ac894e5 100644
@@ -793,6 +793,7 @@ struct netvsc_device {
 
 	/* Receive buffer allocated by us but manages by NetVSP */
 	void *recv_buf;
+	u32 recv_buf_size; /* allocated bytes */
 	u32 recv_buf_gpadl_handle;
 	u32 recv_section_cnt;
 	u32 recv_section_size;

drivers/net/hyperv/netvsc.c +2 -2

index 1ddb2c39b6e4..a6700d65f206 100644
@@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
 		goto cleanup;
 	}
 
+	net_device->recv_buf_size = buf_size;
+
 	/*
 	 * Establish the gpadl handle for this buffer on this
 	 * channel.  Note: This call uses the vmbus connection rather

@@ -1095,11 +1097,22 @@ static int netvsc_receive(struct net_device *ndev,
 
 	/* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
 	for (i = 0; i < count; i++) {
-		void *data = recv_buf
-			+ vmxferpage_packet->ranges[i].byte_offset;

drivers/net/hyperv/netvsc.c static int netvsc_receive(struct net_device *ndev, · line 1109

Quoted 10 lines

drivers/net/hyperv/netvsc.c +7 -0

+		u32 offset = vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		void *data;
 		int ret;
 
+		if (unlikely(offset + buflen > net_device->recv_buf_size)) {
+			status = NVSP_STAT_FAIL;
+			netif_err(net_device_ctx, rx_err, ndev,
+				  "Packet offset:%u + len:%u too big\n",
+				  offset, buflen);

This shouldn't happen, of course, but I'd rather ratelimit this error or even used something like netdev_WARN_ONCE().

Quoted 9 lines

drivers/net/hyperv/netvsc.c +6 -0

+
+			continue;
+		}
+
+		data = recv_buf + offset;
+
 		trace_rndis_recv(ndev, q_idx, data);
 
 		/* Pass it to the upper layer */

Signature · 5 lines

  Vitaly
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Haiyang Zhang

maintainer

2018-03-23 15:25 UTC #6 lore raw

RE: [PATCH net-next,2/2] hv_netvsc: Add range checking for rx packet offset and length

Quoted 64 lines

-----Original Message-----
From: Vitaly Kuznetsov <[email protected]>
Sent: Friday, March 23, 2018 11:17 AM
To: Haiyang Zhang <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; KY Srinivasan <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH net-next,2/2] hv_netvsc: Add range checking for rx packet offset and length
Quoted 49 lines · Haiyang Zhang
From: Haiyang Zhang <[email protected]>
This patch adds range checking for rx packet offset and length. It may only happen if there is a host side bug.
Signed-off-by Haiyang Zhang <[email protected]>
---
 drivers/net/hyperv/hyperv_net.h |  1 +
 drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/net/hyperv/hyperv_net.h index 0db3bd1ea06f..49c05ac894e5 100644
drivers/net/hyperv/hyperv_net.h +1 -0
@@ -793,6 +793,7 @@ struct netvsc_device {
 
 	/* Receive buffer allocated by us but manages by NetVSP */
 	void *recv_buf;
+	u32 recv_buf_size; /* allocated bytes */
 	u32 recv_buf_gpadl_handle;
 	u32 recv_section_cnt;
 	u32 recv_section_size;
drivers/net/hyperv/netvsc.c +2 -1
index 1ddb2c39b6e4..a6700d65f206 100644
@@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
 		goto cleanup;
 	}
 
+	net_device->recv_buf_size = buf_size;
+
 	/*
 	 * Establish the gpadl handle for this buffer on this
 	 * channel.  Note: This call uses the vmbus connection rather @@
-1095,11 +1097,22 @@ static int netvsc_receive(struct net_device
*ndev,
drivers/net/hyperv/netvsc.c +2 -2
 	/* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
 	for (i = 0; i < count; i++) {
-		void *data = recv_buf
-			+ vmxferpage_packet->ranges[i].byte_offset;
+		u32 offset = vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		void *data;
 		int ret;

Quoted 8 lines

drivers/net/hyperv/netvsc.c static int netvsc_receive(struct net_device *ndev, · line 1109
Quoted 5 lines
drivers/net/hyperv/netvsc.c +5 -0
+		if (unlikely(offset + buflen > net_device->recv_buf_size)) {
+			status = NVSP_STAT_FAIL;
+			netif_err(net_device_ctx, rx_err, ndev,
+				  "Packet offset:%u + len:%u too big\n",
+				  offset, buflen);
This shouldn't happen, of course, but I'd rather ratelimit this error or even used something like netdev_WARN_ONCE().

Actually I thought about ratelimit, but this range check is only to catch host side bug. It should not happen.
But if it happens, the VM should not be used anymore. And we need to debug the host. Similarly, some other this kind of checks in the same function are not using ratelimit:

if (unlikely(nvsp->hdr.msg_type != NVSP_MSG1_TYPE_SEND_RNDIS_PKT)) {
netif_err(net_device_ctx, rx_err, ndev,
"Unknown nvsp packet type received %u\n", nvsp->hdr.msg_type);

Thanks,
- Haiyang

Stephen Hemminger

2018-03-27 15:22 UTC #7 lore raw

Re: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet offset and length

In reply to Haiyang Zhang

Quoted 46 lines · On Thu, 22 Mar 2018 12:01:14 -0700 Haiyang Zhang <[email protected]>…

From: Haiyang Zhang <[email protected]>

This patch adds range checking for rx packet offset and length. It may only happen if there is a host side bug.

Signed-off-by Haiyang Zhang <[email protected]>

---
 drivers/net/hyperv/hyperv_net.h |  1 +
 drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

drivers/net/hyperv/hyperv_net.h +1 -0

index 0db3bd1ea06f..49c05ac894e5 100644
@@ -793,6 +793,7 @@ struct netvsc_device {
 
 	/* Receive buffer allocated by us but manages by NetVSP */
 	void *recv_buf;
+	u32 recv_buf_size; /* allocated bytes */
 	u32 recv_buf_gpadl_handle;
 	u32 recv_section_cnt;
 	u32 recv_section_size;

drivers/net/hyperv/netvsc.c +4 -2

index 1ddb2c39b6e4..a6700d65f206 100644
@@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
 		goto cleanup;
 	}
 
+	net_device->recv_buf_size = buf_size;
+
 	/*
 	 * Establish the gpadl handle for this buffer on this
 	 * channel.  Note: This call uses the vmbus connection rather

@@ -1095,11 +1097,22 @@ static int netvsc_receive(struct net_device *ndev,
 
 	/* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
 	for (i = 0; i < count; i++) {
-		void *data = recv_buf
-			+ vmxferpage_packet->ranges[i].byte_offset;
+		u32 offset = vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		void *data;
 		int ret;

drivers/net/hyperv/netvsc.c static int netvsc_receive(struct net_device *ndev, · line 1113

Quoted 10 lines

drivers/net/hyperv/netvsc.c +9 -0

 
+		if (unlikely(offset + buflen > net_device->recv_buf_size)) {
+			status = NVSP_STAT_FAIL;
+			netif_err(net_device_ctx, rx_err, ndev,
+				  "Packet offset:%u + len:%u too big\n",
+				  offset, buflen);
+
+			continue;
+		}
+

If one part of the RNDIS packet is wrong then the whole receive buffer is damaged. Just return, don't continue.

It could really just be a statistic and a one shot log message.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Haiyang Zhang

maintainer

2018-03-27 15:35 UTC #8 lore raw

RE: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet offset and length

Quoted 78 lines

-----Original Message-----
From: Stephen Hemminger <[email protected]>
Sent: Tuesday, March 27, 2018 11:23 AM
To: Haiyang Zhang <[email protected]>
Cc: Haiyang Zhang <[email protected]>; [email protected]; [email protected]; [email protected]; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet offset and length
Quoted 48 lines · On Thu, 22 Mar 2018 12:01:14 -0700 Haiyang Zhang <[email protected]>…
From: Haiyang Zhang <[email protected]>
This patch adds range checking for rx packet offset and length. It may only happen if there is a host side bug.
Signed-off-by Haiyang Zhang <[email protected]>
---
 drivers/net/hyperv/hyperv_net.h |  1 +
 drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/net/hyperv/hyperv_net.h index 0db3bd1ea06f..49c05ac894e5 100644
drivers/net/hyperv/hyperv_net.h +1 -0
@@ -793,6 +793,7 @@ struct netvsc_device {
 
 	/* Receive buffer allocated by us but manages by NetVSP */
 	void *recv_buf;
+	u32 recv_buf_size; /* allocated bytes */
 	u32 recv_buf_gpadl_handle;
 	u32 recv_section_cnt;
 	u32 recv_section_size;
drivers/net/hyperv/netvsc.c +2 -1
index 1ddb2c39b6e4..a6700d65f206 100644
@@ -289,6 +289,8 @@ static int netvsc_init_buf(struct hv_device *device,
 		goto cleanup;
 	}
 
+	net_device->recv_buf_size = buf_size;
+
 	/*
 	 * Establish the gpadl handle for this buffer on this
 	 * channel.  Note: This call uses the vmbus connection rather @@
-1095,11 +1097,22 @@ static int netvsc_receive(struct net_device
*ndev,
drivers/net/hyperv/netvsc.c +2 -2
 	/* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */
 	for (i = 0; i < count; i++) {
-		void *data = recv_buf
-			+ vmxferpage_packet->ranges[i].byte_offset;
+		u32 offset = vmxferpage_packet->ranges[i].byte_offset;
 		u32 buflen = vmxferpage_packet->ranges[i].byte_count;
+		void *data;
 		int ret;
drivers/net/hyperv/netvsc.c static int netvsc_receive(struct net_device *ndev, · line 1113
Quoted 10 lines
drivers/net/hyperv/netvsc.c +9 -0
 
+		if (unlikely(offset + buflen > net_device->recv_buf_size)) {
+			status = NVSP_STAT_FAIL;
+			netif_err(net_device_ctx, rx_err, ndev,
+				  "Packet offset:%u + len:%u too big\n",
+				  offset, buflen);
+
+			continue;
+		}
+
If one part of the RNDIS packet is wrong then the whole receive buffer is damaged. Just return, don't continue.

Quoted 1 line

It could really just be a statistic and a one shot log message.

I will let the loop terminates and send NVSP status fail to the host.

For statistics, this range check is to catch potential host side issues, just like these checks in the same function earlier:

	/* Make sure this is a valid nvsp packet */
	if (unlikely(nvsp->hdr.msg_type != NVSP_MSG1_TYPE_SEND_RNDIS_PKT)) {
		netif_err(net_device_ctx, rx_err, ndev,
			  "Unknown nvsp packet type received %u\n",
			  nvsp->hdr.msg_type);
		return 0;
	}

	if (unlikely(vmxferpage_packet->xfer_pageset_id != NETVSC_RECEIVE_BUFFER_ID)) {
		netif_err(net_device_ctx, rx_err, ndev,
			  "Invalid xfer page set id - expecting %x got %x\n",
			  NETVSC_RECEIVE_BUFFER_ID,
			  vmxferpage_packet->xfer_pageset_id);
		return 0;
	}

If these kinds of errors need statistics, there will be many stat variables... Maybe we should just create one stat variable for all of the "invalid format from host"?

Thanks,
- Haiyang

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

David Miller

maintainer

2018-03-25 21:08 UTC #9 lore raw

In reply to Haiyang Zhang

From: Haiyang Zhang <[email protected]>
Date: Thu, 22 Mar 2018 12:01:12 -0700

Quoted 2 lines

Fix the status code returned to the host. Also add range check for rx packet offset and length.

Series applied, thank you.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel