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 <[email protected]>
> Cc: Saeed Mahameed <[email protected]>
> Signed-off-by: Cong Wang <[email protected]>
> ---
> 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)
>
