[PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani]

In 93515d53b1
  "net: move vlan pop/push functions into common code"
skb_vlan_pop was moved from its private location in openvswitch to
skbuff common code.

In case !vlan_tx_tag_present, the original 'pop_vlan()' assured
that skb->len is sufficient for the existence of a vlan_ethhdr
(if skb->len < VLAN_ETH_HLEN then pop was a no-op).

This validation was moved as is into the new common 'skb_vlan_pop'.

Alas, in its original location (openvswitch), there's a guarantee that
'data' points to the mac_header, therefore the 'skb->len < VLAN_ETH_HLEN'
condition made sense.
However there's no such guarantee in the generic 'skb_vlan_pop'.

For short packets received in rx path going through 'skb_vlan_pop',
this causes 'skb_vlan_pop' to fail pop-ing a valid vlan hdr (in case tag
is in payload), or to fail moving next tag into hw-accel tag.

Instead, verify that 'skb->mac_len' is sufficient.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
 Spotted by code review while doing work augmenting tc act vlan.

 net/core/skbuff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1e329d4112..cc2c004838 100644
--- a/net/core/skbuff.c
+++ 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))
 			return 0;
 
 		err = __skb_vlan_pop(skb, &vlan_tci);
@@ -4547,7 +4547,7 @@ int skb_vlan_pop(struct sk_buff *skb)
 	/* move next vlan tag to hw accel tag */
 	if (likely((skb->protocol != htons(ETH_P_8021Q) &&
 		    skb->protocol != htons(ETH_P_8021AD)) ||
-		   skb->len < VLAN_ETH_HLEN))
+		   skb->mac_len < VLAN_ETH_HLEN))
 		return 0;
 
 	vlan_proto = skb->protocol;
-- 
2.7.4

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[pravin shelar]

On Sun, Sep 18, 2016 at 3:09 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> In 93515d53b1
>   "net: move vlan pop/push functions into common code"
> skb_vlan_pop was moved from its private location in openvswitch to
> skbuff common code.
>
> In case !vlan_tx_tag_present, the original 'pop_vlan()' assured
> that skb->len is sufficient for the existence of a vlan_ethhdr
> (if skb->len < VLAN_ETH_HLEN then pop was a no-op).
>
> This validation was moved as is into the new common 'skb_vlan_pop'.
>
> Alas, in its original location (openvswitch), there's a guarantee that
> 'data' points to the mac_header, therefore the 'skb->len < VLAN_ETH_HLEN'
> condition made sense.
> However there's no such guarantee in the generic 'skb_vlan_pop'.
>
> For short packets received in rx path going through 'skb_vlan_pop',
> this causes 'skb_vlan_pop' to fail pop-ing a valid vlan hdr (in case tag
> is in payload), or to fail moving next tag into hw-accel tag.
>
> Instead, verify that 'skb->mac_len' is sufficient.
>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> ---
>  Spotted by code review while doing work augmenting tc act vlan.
>
>  net/core/skbuff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 1e329d4112..cc2c004838 100644
> --- a/net/core/skbuff.c
> +++ 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.

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani]

Hi,

On Sun, 18 Sep 2016 13:26:30 -0700, pshelar@ovn.org wrote:
> On Sun, Sep 18, 2016 at 3:09 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 1e329d4112..cc2c004838 100644
> > --- a/net/core/skbuff.c
> > +++ 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.

Seems validation in '__skb_vlan_pop' has slightly different semantics:

	unsigned int offset = skb->data - skb_mac_header(skb);

	__skb_push(skb, offset);
	err = skb_ensure_writable(skb, VLAN_ETH_HLEN);

this pushes 'data' back to mac_header, then makes sure there's sufficient
place in skb to _store_ VLAN_ETH_HLEN bytes (by pulling into linear part
if needed, or erroring if skb is too small).

There's no guarantee the original mac header was sized VLAN_ETH_HLEN.

Interpretation of the following 

		if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
			      skb->protocol != htons(ETH_P_8021AD)) ||
			     skb->len < VLAN_ETH_HLEN))
			return 0;

in 'skb_vlan_pop' might be read as:
"there's no tag, or protocol says its a tag but it's insufficient to pop,
 so lets do nothing".

Is it superflous?

Thanks,
Shmulik

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Daniel Borkmann]

On 09/19/2016 08:15 AM, Shmulik Ladkani wrote:
> On Sun, 18 Sep 2016 13:26:30 -0700, pshelar@ovn.org wrote:
>> On Sun, Sep 18, 2016 at 3:09 AM, Shmulik Ladkani
>> <shmulik.ladkani@gmail.com> wrote:
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index 1e329d4112..cc2c004838 100644
>>> --- a/net/core/skbuff.c
>>> +++ 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.
>
> Seems validation in '__skb_vlan_pop' has slightly different semantics:
>
> 	unsigned int offset = skb->data - skb_mac_header(skb);
>
> 	__skb_push(skb, offset);
> 	err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>
> this pushes 'data' back to mac_header, then makes sure there's sufficient
> place in skb to _store_ VLAN_ETH_HLEN bytes (by pulling into linear part
> if needed, or erroring if skb is too small).

Yes, but this skb_ensure_writable() is needed for doing the memmove anyway.

> There's no guarantee the original mac header was sized VLAN_ETH_HLEN.

I'm wondering, what happens when you'd call this on tx path, when you'd
change that to suggested skb->mac_len? Isn't that 0 in such case, thus
such setups could fail then?

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani]

On Mon, 19 Sep 2016 14:22:57 +0200, daniel@iogearbox.net wrote:
> On 09/19/2016 08:15 AM, Shmulik Ladkani wrote:
> > On Sun, 18 Sep 2016 13:26:30 -0700, pshelar@ovn.org wrote:
> >> On Sun, Sep 18, 2016 at 3:09 AM, Shmulik Ladkani
> >> <shmulik.ladkani@gmail.com> wrote:
> >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >>> index 1e329d4112..cc2c004838 100644
> >>> --- a/net/core/skbuff.c
> >>> +++ 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.
> >
> > Seems validation in '__skb_vlan_pop' has slightly different semantics:
> >
> > 	unsigned int offset = skb->data - skb_mac_header(skb);
> >
> > 	__skb_push(skb, offset);
> > 	err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> >
> > this pushes 'data' back to mac_header, then makes sure there's sufficient
> > place in skb to _store_ VLAN_ETH_HLEN bytes (by pulling into linear part
> > if needed, or erroring if skb is too small).
> 
> Yes, but this skb_ensure_writable() is needed for doing the memmove anyway.

Had no intention dropping the skb_ensure_writable from __skb_vlan_pop :)

> > There's no guarantee the original mac header was sized VLAN_ETH_HLEN.
> 
> I'm wondering, what happens when you'd call this on tx path, when you'd
> change that to suggested skb->mac_len? Isn't that 0 in such case, thus
> such setups could fail then?

Thanks, I think you're right.

Seems __dev_queue_xmit needs a 'skb_reset_mac_len' right after call to
'skb_reset_mac_header' (or maybe call 'skb_reset_mac_len' from within
skb_reset_mac_header?).

Also, I'm okay with removing the excess 'skb->mac_len < VLAN_ETH_HLEN'
condition if it is agreed that the "tag exists but insufficient to pop"
semantic is no longer wanted.

Regards,
Shmulik

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani]

On Mon, 19 Sep 2016 16:05:17 +0300 Shmulik Ladkani <shmulik.ladkani@gmail.com> wrote:
> Also, I'm okay with removing the excess 'skb->mac_len < VLAN_ETH_HLEN'
> condition if it is agreed that the "tag exists but insufficient to pop"
> semantic is no longer wanted.

Thinking this over, the condition is indeed superflous, will send a v2.

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani]

Hi Pravin,

On Sun, 18 Sep 2016 13:26:30 -0700 pravin shelar <pshelar@ovn.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

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[pravin shelar]

On Mon, Sep 19, 2016 at 1:04 PM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> Hi Pravin,
>
> On Sun, 18 Sep 2016 13:26:30 -0700 pravin shelar <pshelar@ovn.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?
>
I think this is correct behavior over existing code. And under memory
pressure chances of packet drop are higher even without the change
anyways.

Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani]

Hi,

On Mon, 19 Sep 2016 13:46:10 -0700 pravin shelar <pshelar@ovn.org> wrote:
> On Mon, Sep 19, 2016 at 1:04 PM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > Hi Pravin,
> >
> > On Sun, 18 Sep 2016 13:26:30 -0700 pravin shelar <pshelar@ovn.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?
> >  
> I think this is correct behavior over existing code.

Ok.
I'll submit a v3 identical to v2 but with proper statement of this
behavior change in the commit log.

Thanks.