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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <955711f0-eb24-4512-c9a3-429bdad8bcad@mellanox.com>
Date:   Sun, 2 Dec 2018 08:56:43 +0000
From:   Tariq Toukan <tariqt@...lanox.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [Patch net] mlx5: check for malformed packets



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@...lanox.com>
> Cc: Saeed Mahameed <saeedm@...lanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.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)
>