[Patch net] mlx5: check for malformed packets

[Patch net] mlx5: check for malformed packets

[Cong Wang]

is_last_ethertype_ip() is used to check IP/IPv6 protocol before
parsing IP/IPv6 headers.

But __vlan_get_protocol() is only bound to skb->len, a malicious
packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD
headers, and it may not even contain an IP/IPv6 header at all, so we
have to check if we are still safe to continue to parse IP/IPv6 header.
If not, treat it as non-IP packet.

This should not cause any crash as we stil have tail room in skb,
but we can't just rely on it either.

Cc: Tariq Toukan <tariqt@mellanox.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 624eed345b5d..1e505013ebfd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -693,7 +693,18 @@ static inline bool is_last_ethertype_ip(struct sk_buff *skb, int *network_depth,
 {
 	*proto = ((struct ethhdr *)skb->data)->h_proto;
 	*proto = __vlan_get_protocol(skb, *proto, network_depth);
-	return (*proto == htons(ETH_P_IP) || *proto == htons(ETH_P_IPV6));
+
+	if (*proto == htons(ETH_P_IP)) {
+		if (unlikely(*network_depth > skb->len - sizeof(struct iphdr)))
+			return false;
+		return true;
+	} else if (*proto == htons(ETH_P_IPV6)) {
+		if (unlikely(*network_depth > skb->len - sizeof(struct ipv6hdr)))
+			return false;
+		return true;
+	}
+
+	return false;
 }
 
 static inline void mlx5e_enable_ecn(struct mlx5e_rq *rq, struct sk_buff *skb)
-- 
2.19.1

Re: [Patch net] mlx5: check for malformed packets

[Tariq Toukan]

On 01/12/2018 10:38 PM, Cong Wang wrote:
> is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> parsing IP/IPv6 headers.
> 
> But __vlan_get_protocol() is only bound to skb->len, a malicious
> packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD
> headers, and it may not even contain an IP/IPv6 header at all, so we
> have to check if we are still safe to continue to parse IP/IPv6 header.
> If not, treat it as non-IP packet.
> 
> This should not cause any crash as we stil have tail room in skb,
> but we can't just rely on it either.
> 
> Cc: Tariq Toukan <tariqt@mellanox.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 

Hi Cong,
Thanks for your patch.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 624eed345b5d..1e505013ebfd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -693,7 +693,18 @@ static inline bool is_last_ethertype_ip(struct sk_buff *skb, int *network_depth,
>   {
>   	*proto = ((struct ethhdr *)skb->data)->h_proto;
>   	*proto = __vlan_get_protocol(skb, *proto, network_depth);
> -	return (*proto == htons(ETH_P_IP) || *proto == htons(ETH_P_IPV6));
> +
> +	if (*proto == htons(ETH_P_IP)) {
> +		if (unlikely(*network_depth > skb->len - sizeof(struct iphdr)))
> +			return false;
> +		return true;

Or just do the following?
	return *network_depth <= skb->len - sizeof(struct iphdr));

We'll lose the compiler hint though, so I'm not sure which is better.

> +	} else if (*proto == htons(ETH_P_IPV6)) {

No need for an else here, the first if block always returns.

> +		if (unlikely(*network_depth > skb->len - sizeof(struct ipv6hdr)))
> +			return false;
> +		return true;

Same applies here.

> +	}
> +
> +	return false;
>   }
>   
>   static inline void mlx5e_enable_ecn(struct mlx5e_rq *rq, struct sk_buff *skb)
> 

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Sun, Dec 2, 2018 at 12:56 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>
>
>
> On 01/12/2018 10:38 PM, Cong Wang wrote:
> > +     if (*proto == htons(ETH_P_IP)) {
> > +             if (unlikely(*network_depth > skb->len - sizeof(struct iphdr)))
> > +                     return false;
> > +             return true;
>
> Or just do the following?
>         return *network_depth <= skb->len - sizeof(struct iphdr));
>
> We'll lose the compiler hint though, so I'm not sure which is better.


It is very important to keep this unlikely(), as it is on a hot path.


>
> > +     } else if (*proto == htons(ETH_P_IPV6)) {
>
> No need for an else here, the first if block always returns.


Yeah, but not sure if this makes a difference on the generated
asm code. I will give it a try anyway.

Thanks.

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Sun, Dec 2, 2018 at 9:11 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Dec 2, 2018 at 12:56 AM Tariq Toukan <tariqt@mellanox.com> wrote:
> >
> > > +     } else if (*proto == htons(ETH_P_IPV6)) {
> >
> > No need for an else here, the first if block always returns.
>
>
> Yeah, but not sure if this makes a difference on the generated
> asm code. I will give it a try anyway.

I just tried, there is no difference on the generated assembly code.

The gcc I use is:

$ gcc --version
gcc (GCC) 8.2.1 20181011 (Red Hat 8.2.1-4)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Are you okay with the current code now? :)

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Sat, Dec 1, 2018 at 12:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> parsing IP/IPv6 headers.

One thing I noticed while reviewing the assembly code is that
is_last_ethertype_ip() is no longer inlined after this patch.

I think I should keep it inlined by using __always_inline, as it is on
a hot path.

What do you think, Tariq?

Re: [Patch net] mlx5: check for malformed packets

[Saeed Mahameed]

On Mon, Dec 3, 2018 at 10:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sat, Dec 1, 2018 at 12:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> > parsing IP/IPv6 headers.
>
> One thing I noticed while reviewing the assembly code is that
> is_last_ethertype_ip() is no longer inlined after this patch.
>
> I think I should keep it inlined by using __always_inline, as it is on
> a hot path.
>
> What do you think, Tariq?

We are against having inline keywords in c file,  so better if you
move this function to a header file an force the inline keyword there.

Re: [Patch net] mlx5: check for malformed packets

[Saeed Mahameed]

On Sat, Dec 1, 2018 at 12:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> parsing IP/IPv6 headers.
>
> But __vlan_get_protocol() is only bound to skb->len, a malicious
> packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD
> headers, and it may not even contain an IP/IPv6 header at all, so we
> have to check if we are still safe to continue to parse IP/IPv6 header.
> If not, treat it as non-IP packet.
>
> This should not cause any crash as we stil have tail room in skb,
> but we can't just rely on it either.

Hi Cong, is this reproducible or just a theory ? which part of the
code you think will cause the invalid access or crash ?
do you have steps to reproduce this?

I would like to investigate this myself, it will take a couple of days
if that's ok with you ..

Thanks,
Saeed.

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Tue, Dec 4, 2018 at 11:28 AM Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
>
> We are against having inline keywords in c file,  so better if you
> move this function to a header file an force the inline keyword there.


Two points:

1. The existing code without my patch has inline keyword. Why
not move it from the beginning?

2. As I already mentioned, inline doesn't work with my patch,
__always_inline does.

Therefore, moving inline to a header doesn't make any difference.

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Tue, Dec 4, 2018 at 11:33 AM Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
>
> On Sat, Dec 1, 2018 at 12:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> > parsing IP/IPv6 headers.
> >
> > But __vlan_get_protocol() is only bound to skb->len, a malicious
> > packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD
> > headers, and it may not even contain an IP/IPv6 header at all, so we
> > have to check if we are still safe to continue to parse IP/IPv6 header.
> > If not, treat it as non-IP packet.
> >
> > This should not cause any crash as we stil have tail room in skb,
> > but we can't just rely on it either.
>
> Hi Cong, is this reproducible or just a theory ? which part of the
> code you think will cause the invalid access or crash ?

Since you don't even read into my changelog, here it is:

"This should not cause any crash as we stil have tail room in skb,
but we can't just rely on it either."

As I already explained to you in a private email, when we
reference whatever field in struct iphdr, we have to make sure
the offset of that field is within skb->len.


> do you have steps to reproduce this?
>

Again, you really have to read the changelog I wrote:


"a malicious
packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD
headers, and it may not even contain an IP/IPv6 header at all, "


> I would like to investigate this myself, it will take a couple of days
> if that's ok with you ..

Sure, take your time. I am sending the patch only for showing
the problem, NOT to merge.


Let's discard it anyway. I am wasting my time.

Thanks.

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Sat, Dec 1, 2018 at 12:38 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> parsing IP/IPv6 headers.
>
> But __vlan_get_protocol() is only bound to skb->len, a malicious
> packet could exhaust all skb->len by inserting sufficient ETH_P_8021AD
> headers, and it may not even contain an IP/IPv6 header at all, so we
> have to check if we are still safe to continue to parse IP/IPv6 header.
> If not, treat it as non-IP packet.
>
> This should not cause any crash as we stil have tail room in skb,
> but we can't just rely on it either.
>
> Cc: Tariq Toukan <tariqt@mellanox.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

NAcked-by: Cong Wang <xiyou.wangcong@gmail.com>

This patch has no value for upstream. Let's discard it.

Thanks!

Re: [Patch net] mlx5: check for malformed packets

[Saeed Mahameed]

On Tue, 2018-12-04 at 12:21 -0800, Cong Wang wrote:
> On Tue, Dec 4, 2018 at 11:33 AM Saeed Mahameed
> <saeedm@dev.mellanox.co.il> wrote:
> > On Sat, Dec 1, 2018 at 12:38 PM Cong Wang <xiyou.wangcong@gmail.com
> > > wrote:
> > > is_last_ethertype_ip() is used to check IP/IPv6 protocol before
> > > parsing IP/IPv6 headers.
> > > 
> > > But __vlan_get_protocol() is only bound to skb->len, a malicious
> > > packet could exhaust all skb->len by inserting sufficient
> > > ETH_P_8021AD
> > > headers, and it may not even contain an IP/IPv6 header at all, so
> > > we
> > > have to check if we are still safe to continue to parse IP/IPv6
> > > header.
> > > If not, treat it as non-IP packet.
> > > 
> > > This should not cause any crash as we stil have tail room in skb,
> > > but we can't just rely on it either.
> > 
> > Hi Cong, is this reproducible or just a theory ? which part of the
> > code you think will cause the invalid access or crash ?
> 
> Since you don't even read into my changelog, here it is:
> 
> "This should not cause any crash as we stil have tail room in skb,
> but we can't just rely on it either."
> 
> As I already explained to you in a private email, when we
> reference whatever field in struct iphdr, we have to make sure
> the offset of that field is within skb->len.
> 
> 
> > do you have steps to reproduce this?
> > 
> 
> Again, you really have to read the changelog I wrote:
> 
> 
> "a malicious
> packet could exhaust all skb->len by inserting sufficient
> ETH_P_8021AD
> headers, and it may not even contain an IP/IPv6 header at all, "
> 

I read it and i understood it, i was just wondering if you are actually
able to reproduce it, and if you have the command line steps to share
with us.

> 
> > I would like to investigate this myself, it will take a couple of
> > days
> > if that's ok with you ..
> 
> Sure, take your time. I am sending the patch only for showing
> the problem, NOT to merge.
> 
> 
> Let's discard it anyway. I am wasting my time.

Ok, will be able to start looking at this in a couple of days, sorry
about your time and thanks a lot for the report.

Please understand that RX data path is really sensitive and we are
trying to find the optimal fix of any issue here, sorry for any
inconvenience.

> 
> Thanks.

Re: [Patch net] mlx5: check for malformed packets

[Cong Wang]

On Tue, Dec 4, 2018 at 5:16 PM Saeed Mahameed <saeedm@mellanox.com> wrote:
> Please understand that RX data path is really sensitive and we are
> trying to find the optimal fix of any issue here, sorry for any
> inconvenience.

I am sorry for sending out this patch, I am certain that it wastes
a lot of your time.

The best we can do is to just ignore it.

Thanks!