diff mbox series

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

Message ID 20180322190114.25596-3-haiyangz@linuxonhyperv.com
State Accepted, archived
Delegated to: David Miller
Headers show
Series hv_netvsc: Fix/improve RX path error handling | expand

Commit Message

Haiyang Zhang March 22, 2018, 7:01 p.m. UTC 
From: Haiyang Zhang <haiyangz@microsoft.com>

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 <haiyangz@microsoft.com>
---
 drivers/net/hyperv/hyperv_net.h |  1 +
 drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

Comments

Vitaly Kuznetsov March 23, 2018, 3:17 p.m. UTC | #1 
Haiyang Zhang <haiyangz@linuxonhyperv.com> writes:

> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> 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 <haiyangz@microsoft.com>
> ---
>  drivers/net/hyperv/hyperv_net.h |  1 +
>  drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index 0db3bd1ea06f..49c05ac894e5 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -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;
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 1ddb2c39b6e4..a6700d65f206 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -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);

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

> +
> +			continue;
> +		}
> +
> +		data = recv_buf + offset;
> +
>  		trace_rndis_recv(ndev, q_idx, data);
>
>  		/* Pass it to the upper layer */
Haiyang Zhang March 23, 2018, 3:25 p.m. UTC | #2 
> -----Original Message-----
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Friday, March 23, 2018 11:17 AM
> To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Haiyang Zhang
> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de;
> devel@linuxdriverproject.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next,2/2] hv_netvsc: Add range checking for rx packet
> offset and length
> 
> Haiyang Zhang <haiyangz@linuxonhyperv.com> writes:
> 
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > 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 <haiyangz@microsoft.com>
> > ---
> >  drivers/net/hyperv/hyperv_net.h |  1 +
> >  drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/hyperv_net.h
> > b/drivers/net/hyperv/hyperv_net.h index 0db3bd1ea06f..49c05ac894e5
> > 100644
> > --- a/drivers/net/hyperv/hyperv_net.h
> > +++ b/drivers/net/hyperv/hyperv_net.h
> > @@ -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;
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 1ddb2c39b6e4..a6700d65f206 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -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);
> 
> 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 March 27, 2018, 3:22 p.m. UTC | #3 
On Thu, 22 Mar 2018 12:01:14 -0700
Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:

> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> 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 <haiyangz@microsoft.com>
> ---
>  drivers/net/hyperv/hyperv_net.h |  1 +
>  drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index 0db3bd1ea06f..49c05ac894e5 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -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;
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 1ddb2c39b6e4..a6700d65f206 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -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;
> +		}
> +

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.
Haiyang Zhang March 27, 2018, 3:35 p.m. UTC | #4 
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, March 27, 2018 11:23 AM
> To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net;
> netdev@vger.kernel.org; olaf@aepfle.de; Stephen Hemminger
> <sthemmin@microsoft.com>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; vkuznets@redhat.com
> Subject: Re: [PATCH net-next, 2/2] hv_netvsc: Add range checking for rx packet
> offset and length
> 
> On Thu, 22 Mar 2018 12:01:14 -0700
> Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
> 
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > 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 <haiyangz@microsoft.com>
> > ---
> >  drivers/net/hyperv/hyperv_net.h |  1 +
> >  drivers/net/hyperv/netvsc.c     | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/hyperv_net.h
> > b/drivers/net/hyperv/hyperv_net.h index 0db3bd1ea06f..49c05ac894e5
> > 100644
> > --- a/drivers/net/hyperv/hyperv_net.h
> > +++ b/drivers/net/hyperv/hyperv_net.h
> > @@ -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;
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 1ddb2c39b6e4..a6700d65f206 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -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;
> > +		}
> > +
> 
> 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.

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
diff mbox series

Patch

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 0db3bd1ea06f..49c05ac894e5 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -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;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1ddb2c39b6e4..a6700d65f206 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -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 */