[PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().

[PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().

[Michael Chan]

There have been reports of oversize UDP packets being sent to the
driver to be transmitted, causing error conditions.  The issue is
likely caused by the dst of the SKB switching between 'lo' with
64K MTU and the hardware device with a smaller MTU.  Patches are
being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
issue.

Separately, we should add a length check in validate_xmit_skb()
to drop these oversize packets before they reach the driver.
This patch only validates non-TSO packets.  Complete validation
of segmented TSO packet size will probably be too slow.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 net/core/dev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5d03889..50c5174 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3373,6 +3373,13 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
 		}
 	}
 
+	if (!skb_is_gso(skb) &&
+	    skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)) {
+		net_warn_ratelimited("%s(): Dropping %d bytes oversize skb.\n",
+				     __func__, skb->len);
+		goto out_kfree_skb;
+	}
+
 	skb = validate_xmit_xfrm(skb, features, again);
 
 	return skb;
-- 
2.5.1

Re: [PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().

[Eric Dumazet]

On 02/26/2019 02:56 AM, Michael Chan wrote:
> There have been reports of oversize UDP packets being sent to the
> driver to be transmitted, causing error conditions.  The issue is
> likely caused by the dst of the SKB switching between 'lo' with
> 64K MTU and the hardware device with a smaller MTU.  Patches are
> being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
> issue.
> 
> Separately, we should add a length check in validate_xmit_skb()
> to drop these oversize packets before they reach the driver.

Why ?

We keep adding checks in the 'fast path' and make slower and slower after each release.

We need to fix the root cause really.

> This patch only validates non-TSO packets.  Complete validation
> of segmented TSO packet size will probably be too slow.
> 
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  net/core/dev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5d03889..50c5174 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3373,6 +3373,13 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
>  		}
>  	}
>  
> +	if (!skb_is_gso(skb) &&
> +	    skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)) {
> +		net_warn_ratelimited("%s(): Dropping %d bytes oversize skb.\n",
> +				     __func__, skb->len);
> +		goto out_kfree_skb;
> +	}
> +
>  	skb = validate_xmit_xfrm(skb, features, again);
>  
>  	return skb;
> 

Re: [PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Tue, 26 Feb 2019 05:56:41 -0500

> There have been reports of oversize UDP packets being sent to the
> driver to be transmitted, causing error conditions.  The issue is
> likely caused by the dst of the SKB switching between 'lo' with
> 64K MTU and the hardware device with a smaller MTU.  Patches are
> being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
> issue.
> 
> Separately, we should add a length check in validate_xmit_skb()
> to drop these oversize packets before they reach the driver.
> This patch only validates non-TSO packets.  Complete validation
> of segmented TSO packet size will probably be too slow.
> 
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Anything which changes the dst of an SKB really is responsible for
fixing up whatever became "incompatible" in the new path.

So like Eric I want to see this out of the fast path.

Re: [PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().

[Michael Chan]

On Tue, Feb 26, 2019 at 9:13 AM David Miller <davem@davemloft.net> wrote:
>
> From: Michael Chan <michael.chan@broadcom.com>
> Date: Tue, 26 Feb 2019 05:56:41 -0500
>
> > There have been reports of oversize UDP packets being sent to the
> > driver to be transmitted, causing error conditions.  The issue is
> > likely caused by the dst of the SKB switching between 'lo' with
> > 64K MTU and the hardware device with a smaller MTU.  Patches are
> > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
> > issue.
> >
> > Separately, we should add a length check in validate_xmit_skb()
> > to drop these oversize packets before they reach the driver.
> > This patch only validates non-TSO packets.  Complete validation
> > of segmented TSO packet size will probably be too slow.
> >
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>
> Anything which changes the dst of an SKB really is responsible for
> fixing up whatever became "incompatible" in the new path.
>
> So like Eric I want to see this out of the fast path.

Ok.  In the meantime, will you take a 2-line bnxt_en patch that will
prevent this issue in kernel 5.0?

Re: [PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Tue, 26 Feb 2019 10:22:42 -0800

> On Tue, Feb 26, 2019 at 9:13 AM David Miller <davem@davemloft.net> wrote:
>>
>> From: Michael Chan <michael.chan@broadcom.com>
>> Date: Tue, 26 Feb 2019 05:56:41 -0500
>>
>> > There have been reports of oversize UDP packets being sent to the
>> > driver to be transmitted, causing error conditions.  The issue is
>> > likely caused by the dst of the SKB switching between 'lo' with
>> > 64K MTU and the hardware device with a smaller MTU.  Patches are
>> > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
>> > issue.
>> >
>> > Separately, we should add a length check in validate_xmit_skb()
>> > to drop these oversize packets before they reach the driver.
>> > This patch only validates non-TSO packets.  Complete validation
>> > of segmented TSO packet size will probably be too slow.
>> >
>> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>
>> Anything which changes the dst of an SKB really is responsible for
>> fixing up whatever became "incompatible" in the new path.
>>
>> So like Eric I want to see this out of the fast path.
> 
> Ok.  In the meantime, will you take a 2-line bnxt_en patch that will
> prevent this issue in kernel 5.0?

Sure, but we will have to remember to remove it when it is no longer
necessary...