netdev - Re: [PATCH] net: skbuff: Fix length validation in skb_vlan

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]

Date:   Mon, 19 Sep 2016 23:04:54 +0300
From:   Shmulik Ladkani <shmulik.ladkani@...il.com>
To:     pravin shelar <pshelar@....org>
Cc:     Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

Hi Pravin,

On Sun, 18 Sep 2016 13:26:30 -0700 pravin shelar <pshelar@....org> wrote:
> > +++ b/net/core/skbuff.c
> > @@ -4537,7 +4537,7 @@ int skb_vlan_pop(struct sk_buff *skb)
> >         } else {
> >                 if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
> >                               skb->protocol != htons(ETH_P_8021AD)) ||
> > -                            skb->len < VLAN_ETH_HLEN))
> > +                            skb->mac_len < VLAN_ETH_HLEN))  
> 
> There is already check in __skb_vlan_pop() to validate skb for a vlan
> header. So it is safe to drop this check entirely.

Yep, I submitted a v2 with your suggestion, however I withdrew it, as
there is a slight behavior difference noticable by 'skb_vlan_pop' callers.

Suppose the rare case where skb->len is too small.

pre:
  skb_vlan_pop returns 0 (at least for the correct tx path).
  Meaning, callers do not see it as a failure.
post:
  skb_ensure_writable fails (!pskb_may_pull), therefore -ENOMEM returned
  to the callers of 'skb_vlan_pop'.

For ovs, it means do_execute_actions's loop is terminated, no further
actions are executed, and skb gets freed.

For tc act vlan, it means skb gets dropped.

This actually makes sense, but do we want to present this change?

Thanks,
Shmulik