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

[Tariq Toukan <tariqt@mellanox.com>]

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)
>