[PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

The contract between the socket layer and the device is that there
will be at least enough bytes in the non-paged part of the skb to
cover a link layer header, and this is ensured by copying any
application provided L2 header into the skb->data and then invoking
dev_validate_header().

If the application has provided fewer than hard_header_len bytes,
dev_validate_header() will zero out the skb->data as needed. This is
acceptable for SOCK_DGRAM/PF_PACKET sockets but in all other cases,
the application must provide a full L2 header, and the PF_PACKET Tx
paths must fail with an error when fewer than hard_header_len bytes
are detected.

All invocations to dev_validate_header() already adjusts the
skb's data, len, tail etc pointers based on hard_header_len before
invoking dev_validate_header(), so additional skb pointers should
not be needed after dev_validate_header().

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 include/linux/netdevice.h |   11 ++++++-----
 net/packet/af_packet.c    |   27 +++++++++++++++++++++------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3868c32..9d49898 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2685,21 +2685,22 @@ static inline int dev_parse_header(const struct sk_buff *skb,
 }
 
 /* ll_header must have at least hard_header_len allocated */
-static inline bool dev_validate_header(const struct net_device *dev,
+static inline int dev_validate_header(const struct net_device *dev,
 				       char *ll_header, int len)
 {
 	if (likely(len >= dev->hard_header_len))
-		return true;
+		return len;
 
 	if (capable(CAP_SYS_RAWIO)) {
 		memset(ll_header + len, 0, dev->hard_header_len - len);
-		return true;
+		return dev->hard_header_len;
 	}
 
 	if (dev->header_ops && dev->header_ops->validate)
-		return dev->header_ops->validate(ll_header, len);
+		if (!dev->header_ops->validate(ll_header, len))
+			return -1;
 
-	return false;
+	return dev->hard_header_len;
 }
 
 typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int len);
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index ddbda25..7af09a3 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1845,6 +1845,7 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
 	__be16 proto = 0;
 	int err;
 	int extra_len = 0;
+	int newlen;
 
 	/*
 	 *	Get and verify the address.
@@ -1920,7 +1921,11 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
 		goto retry;
 	}
 
-	if (!dev_validate_header(dev, skb->data, len)) {
+	newlen = dev_validate_header(dev, skb->data, len);
+	/* As comments above this function indicate, a full L2 header
+	 * must be passed to this function, so if newlen > len, bail.
+	 */
+	if (newlen < 0 || newlen > len) {
 		err = -EINVAL;
 		goto out_unlock;
 	}
@@ -2447,14 +2452,21 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 			return -EINVAL;
 	} else if (copylen) {
 		int hdrlen = min_t(int, copylen, tp_len);
+		int newlen;
 
 		skb_push(skb, dev->hard_header_len);
 		skb_put(skb, copylen - dev->hard_header_len);
 		err = skb_store_bits(skb, 0, data, hdrlen);
 		if (unlikely(err))
 			return err;
-		if (!dev_validate_header(dev, skb->data, hdrlen))
+		newlen = dev_validate_header(dev, skb->data, hdrlen);
+		if (newlen < 0)
 			return -EINVAL;
+		/* Caller has allocated for copylen in non-paged part of
+		 * skb so we should never find newlen > hdrlen
+		 */
+		WARN_ON(newlen > hdrlen);
+
 		if (!skb->protocol)
 			tpacket_set_protocol(dev, skb);
 
@@ -2857,10 +2869,13 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	if (err)
 		goto out_free;
 
-	if (sock->type == SOCK_RAW &&
-	    !dev_validate_header(dev, skb->data, len)) {
-		err = -EINVAL;
-		goto out_free;
+	if (sock->type == SOCK_RAW) {
+		int newlen = dev_validate_header(dev, skb->data, len);
+
+		if (newlen < 0 || newlen > len) {
+			err = -EINVAL;
+			goto out_free;
+		}
 	}
 
 	sock_tx_timestamp(sk, sockc.tsflags, &skb_shinfo(skb)->tx_flags);
-- 
1.7.1

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Tue, 24 Jan 2017 08:11:49 -0800

> @@ -2685,21 +2685,22 @@ static inline int dev_parse_header(const struct sk_buff *skb,
>  }
>  
>  /* ll_header must have at least hard_header_len allocated */
> -static inline bool dev_validate_header(const struct net_device *dev,
> +static inline int dev_validate_header(const struct net_device *dev,
>  				       char *ll_header, int len)
>  {
>  	if (likely(len >= dev->hard_header_len))
> -		return true;
> +		return len;
>  
>  	if (capable(CAP_SYS_RAWIO)) {
>  		memset(ll_header + len, 0, dev->hard_header_len - len);
> -		return true;
> +		return dev->hard_header_len;
>  	}
>  
>  	if (dev->header_ops && dev->header_ops->validate)
> -		return dev->header_ops->validate(ll_header, len);
> +		if (!dev->header_ops->validate(ll_header, len))
> +			return -1;
>  
> -	return false;
> +	return dev->hard_header_len;
>  }
>  
>  typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int len);

This mostly looks good.  But I'm not so sure you handle the variable length header
case properly.  That's why we have the header_ops->validate() callback, to accomodate
that.

In the variable length case, you'll end up having to return something other than
just hard_header_len.  Probably you'll need to make header_ops->validate() return
that length.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

> If the application has provided fewer than hard_header_len bytes,
> dev_validate_header() will zero out the skb->data as needed. This is
> acceptable for SOCK_DGRAM/PF_PACKET sockets but in all other cases,

This was added not for datagram sockets, but to be able to bypass
validation. See the message in commit 2793a23aacbd ("net: validate
variable length ll header") and discussion leading up to that patch.

> the application must provide a full L2 header, and the PF_PACKET Tx
> paths must fail with an error when fewer than hard_header_len bytes
> are detected.

As David pointed out, this does not handle variable length headers
correctly. In link layers that support these, hard_header_len defines
the maximum header length. A hard failure on len < hard_header_len
would be incorrect.

The ->validate callback was added to allow specifying additional
constraints on a per protocol basis. This is where a min constraint
can be added, e.g., for ethernet.

> All invocations to dev_validate_header() already adjusts the
> skb's data, len, tail etc pointers based on hard_header_len before
> invoking dev_validate_header(), so additional skb pointers should
> not be needed after dev_validate_header().
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---

> -       if (!dev_validate_header(dev, skb->data, len)) {
> +       newlen = dev_validate_header(dev, skb->data, len);
> +       /* As comments above this function indicate, a full L2 header
> +        * must be passed to this function, so if newlen > len, bail.
> +        */
> +       if (newlen < 0 || newlen > len) {

If callers only care whether the function returned failure or
increased len, which also indicates failure, it is cleaner to leave it
a boolean and fail in cases where len < the minimum for that link
layer type. No caller actually uses newlen.

> +               /* Caller has allocated for copylen in non-paged part of
> +                * skb so we should never find newlen > hdrlen
> +                */
> +               WARN_ON(newlen > hdrlen);

WARN_ON_ONCE is safer.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/26/17 15:21), Willem de Bruijn wrote:
> > If the application has provided fewer than hard_header_len bytes,
> > dev_validate_header() will zero out the skb->data as needed. This is
> > acceptable for SOCK_DGRAM/PF_PACKET sockets but in all other cases,
> 
> This was added not for datagram sockets, but to be able to bypass
> validation. See the message in commit 2793a23aacbd ("net: validate
> variable length ll header") and discussion leading up to that patch.

some context, I got inot this patch as a result of  the comments in
 https://www.mail-archive.com/netdev@vger.kernel.org/msg149031.html

> As David pointed out, this does not handle variable length headers
> correctly. In link layers that support these, hard_header_len defines
> the maximum header length. A hard failure on len < hard_header_len
> would be incorrect.

right, since DaveM's comments, I took a look at the drivers
that have a ->validate - afaict (from cscope) ax25 is the only 
in-kernel driver that actually passes a ->validate pointer.. 
I tried patching ax25 here:
  http://marc.info/?l=linux-hams&m=148537926422828&w=2
Still waiting to hear back from that list (which doesnt seem to have
much traffic so maybe I should time out on it). Does that
patch make better sense (I'll look up the comments leading up
to 2793a23aacbd later tonight)

> The ->validate callback was added to allow specifying additional
> constraints on a per protocol basis. This is where a min constraint
> can be added, e.g., for ethernet.
> 
> > -       if (!dev_validate_header(dev, skb->data, len)) {
> > +       newlen = dev_validate_header(dev, skb->data, len);
> > +       /* As comments above this function indicate, a full L2 header
> > +        * must be passed to this function, so if newlen > len, bail.
> > +        */
> > +       if (newlen < 0 || newlen > len) {
> 
> If callers only care whether the function returned failure or
> increased len, which also indicates failure, it is cleaner to leave it
> a boolean and fail in cases where len < the minimum for that link
> layer type. No caller actually uses newlen.
> 
> > +               /* Caller has allocated for copylen in non-paged part of
> > +                * skb so we should never find newlen > hdrlen
> > +                */
> > +               WARN_ON(newlen > hdrlen);
> 
> WARN_ON_ONCE is safer.

Ok that's easy enough to do.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

On Thu, Jan 26, 2017 at 4:37 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (01/26/17 15:21), Willem de Bruijn wrote:
>> > If the application has provided fewer than hard_header_len bytes,
>> > dev_validate_header() will zero out the skb->data as needed. This is
>> > acceptable for SOCK_DGRAM/PF_PACKET sockets but in all other cases,
>>
>> This was added not for datagram sockets, but to be able to bypass
>> validation. See the message in commit 2793a23aacbd ("net: validate
>> variable length ll header") and discussion leading up to that patch.
>
> some context, I got inot this patch as a result of  the comments in
>  https://www.mail-archive.com/netdev@vger.kernel.org/msg149031.html
>
>> As David pointed out, this does not handle variable length headers
>> correctly. In link layers that support these, hard_header_len defines
>> the maximum header length. A hard failure on len < hard_header_len
>> would be incorrect.
>
> right, since DaveM's comments, I took a look at the drivers
> that have a ->validate - afaict (from cscope) ax25 is the only
> in-kernel driver that actually passes a ->validate pointer..
> I tried patching ax25 here:
>   http://marc.info/?l=linux-hams&m=148537926422828&w=2
> Still waiting to hear back from that list (which doesnt seem to have
> much traffic so maybe I should time out on it). Does that
> patch make better sense (I'll look up the comments leading up
> to 2793a23aacbd later tonight)

Thanks for the context. ax25_addr_parse doesn't adjust length, it only
verifies that the contents of the variable length header matches
protocol spec. I don't think that it or the .validate callback have to
be modified to return length.

To ensure that skb_headlen(skb) is at least a valid header length even
when CAP_SYS_RAWIO bypasses validation perhaps revise
dev_validate_header to take an additional skb->len parameter and
call skb_put directly from inside that branch.

>> The ->validate callback was added to allow specifying additional
>> constraints on a per protocol basis. This is where a min constraint
>> can be added, e.g., for ethernet.
>>
>> > -       if (!dev_validate_header(dev, skb->data, len)) {
>> > +       newlen = dev_validate_header(dev, skb->data, len);
>> > +       /* As comments above this function indicate, a full L2 header
>> > +        * must be passed to this function, so if newlen > len, bail.
>> > +        */
>> > +       if (newlen < 0 || newlen > len) {
>>
>> If callers only care whether the function returned failure or
>> increased len, which also indicates failure, it is cleaner to leave it
>> a boolean and fail in cases where len < the minimum for that link
>> layer type. No caller actually uses newlen.
>>
>> > +               /* Caller has allocated for copylen in non-paged part of
>> > +                * skb so we should never find newlen > hdrlen
>> > +                */
>> > +               WARN_ON(newlen > hdrlen);
>>
>> WARN_ON_ONCE is safer.
>
> Ok that's easy enough to do.
>

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/26/17 19:08), Willem de Bruijn wrote:
> 
> Thanks for the context. ax25_addr_parse doesn't adjust length, it only
> verifies that the contents of the variable length header matches
> protocol spec. I don't think that it or the .validate callback have to
> be modified to return length.

Yes, I noticed that too, but my reading of ax25_addr_parse
was that it checks to see that a sane L2 header has been 
passed in, and if that (sane-header) is the case, it
returns pointer to the start of data. Thus the returned
(non-null) pointer minus start should tell you the "real"
header length- is my understanding correct?

> To ensure that skb_headlen(skb) is at least a valid header length even
> when CAP_SYS_RAWIO bypasses validation perhaps revise
> dev_validate_header to take an additional skb->len parameter and
> call skb_put directly from inside that branch.

but when I scanned the af_packet code (which appears to
be the only thing that uses dev_validate_header today)
it already sets up the skb->data and ->len pointers up
correctly (based on len, hard_header_len etc) *before*
calling dev_validate_header, so the additional skb_put
is not needed?

still havent googled up prior discussions that led
to dev_validate_header- will probably do that tomorrow AM.

--Sowmini

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

On Thu, Jan 26, 2017 at 9:08 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (01/26/17 19:08), Willem de Bruijn wrote:
>>
>> Thanks for the context. ax25_addr_parse doesn't adjust length, it only
>> verifies that the contents of the variable length header matches
>> protocol spec. I don't think that it or the .validate callback have to
>> be modified to return length.
>
> Yes, I noticed that too, but my reading of ax25_addr_parse
> was that it checks to see that a sane L2 header has been
> passed in, and if that (sane-header) is the case, it
> returns pointer to the start of data. Thus the returned
> (non-null) pointer minus start should tell you the "real"
> header length- is my understanding correct?

Yes. ax25_validate_header reads up to len bytes to parse the header,
so it is smaller than or equal to len or the function returns false.
It is not necessary to check whether the return value exceeds the len
passed to dev_validate_header.

The immediate problem you were facing is that dev_validate_header
accepts values smaller than hard_header_len even for protocols with
fixed header lengths.

This is a consequence of that CAP_SYS_RAWIO branch. Without it,
dev_validate_header would have correctly dropped your packet. That
branch was added because there are tests that explicitly test bad
input. Ideally, it would be behind sysctl and static key, but doing so
might start failing active tests.

See also this discussion

  Sending short raw packets using sendmsg() broke
  https://www.mail-archive.com/netdev@vger.kernel.org/msg99081.html

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/27/17 09:37), Willem de Bruijn wrote:
> The immediate problem you were facing is that dev_validate_header
> accepts values smaller than hard_header_len even for protocols with
> fixed header lengths.

Yes!

> This is a consequence of that CAP_SYS_RAWIO branch. Without it,
> dev_validate_header would have correctly dropped your packet. That
> branch was added because there are tests that explicitly test bad
> input. Ideally, it would be behind sysctl and static key, but doing so
> might start failing active tests.

so this is quite perplexing to someone not familiar with ax25-like
interfaces.  In addition to the pointer you shared, I see
  https://www.spinics.net/lists/netdev/msg367358.html
where the quote is

" The AX.25 device level drivers are simply written to be robust if
  thrown partial frames.
   :
  The other thing that concerns me about this added logic in general is
  that you are also breaking test tools that want to deliberately send
  corrupt frames to certain classes of interface."

But how does the driver (even a robust one!) compute the L2 dst/src if the
application has not even passed down the minimum (which is 21 for ax25?)

Would it make sense to only do the CAP_SYS_RAWIO branch if the 
driver declares itself to have variable length L2 headers, via, e.g.,
some priv flag?

--Sowmini

BTW the http://comments.gmane.org/gmane.linux.network/401064 referred
to in commit 2793a23 is not accessible any more, not sure if its contents
were the same as the link you just shared.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

> " The AX.25 device level drivers are simply written to be robust if
>   thrown partial frames.
>    :
>   The other thing that concerns me about this added logic in general is
>   that you are also breaking test tools that want to deliberately send
>   corrupt frames to certain classes of interface."
>
> But how does the driver (even a robust one!) compute the L2 dst/src if the
> application has not even passed down the minimum (which is 21 for ax25?)

Perhaps the goal is to test that the driver gracefully handles such
packets. I can only speculate.

> Would it make sense to only do the CAP_SYS_RAWIO branch if the
> driver declares itself to have variable length L2 headers, via, e.g.,
> some priv flag?

At the time, the comments were not specific to AX25. Again, we should
probably put that bypass behind a flag, enabling validating in the common case.

> BTW the http://comments.gmane.org/gmane.linux.network/401064 referred
> to in commit 2793a23 is not accessible any more, not sure if its contents
> were the same as the link you just shared.

It is. I looked it up in my email archive. Too bad that that seems to
be the only way.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/27/17 10:28), Willem de Bruijn wrote:
> > Would it make sense to only do the CAP_SYS_RAWIO branch if the
> > driver declares itself to have variable length L2 headers, via, e.g.,
> > some priv flag?
> 
> At the time, the comments were not specific to AX25. Again, we should
> probably put that bypass behind a flag, enabling validating in the common case.

Just to make sure I'm on the same page as you (since you have more
history with this one..) we are going to have a priv_flags like
IFF_VAR_L2HDR which (today) would only be set for ax25, and 
we would only take the CAP_SYS_RAWIO branch for IFF_VAR_L2HDR, right?

--Sowmini

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

> On (01/27/17 10:28), Willem de Bruijn wrote:
>> > Would it make sense to only do the CAP_SYS_RAWIO branch if the
>> > driver declares itself to have variable length L2 headers, via, e.g.,
>> > some priv flag?
>>
>> At the time, the comments were not specific to AX25. Again, we should
>> probably put that bypass behind a flag, enabling validating in the common case.
>
> Just to make sure I'm on the same page as you (since you have more
> history with this one..) we are going to have a priv_flags like
> IFF_VAR_L2HDR which (today) would only be set for ax25, and
> we would only take the CAP_SYS_RAWIO branch for IFF_VAR_L2HDR, right?

I suggested a sysctl. But either approach may cause test applications
that depend on current behavior to start failing.

As your patch state, the contract is that any packet delivered to a
driver has the entire L2 in its linear section. Drivers are not required
to be robust against shorter packets, so there is no reason to test
those.

One option is to limit your fix to known fixed-header protocols.
In these cases hard_header_len is the minimum, so anything
smaller must be dropped.

For protocols with variable header length it is fine to send packets
shorter than hard_header_len, even with corrupted content (i.e.,
even if they would fail that protocol's validate callback), as long as
they exceed the minimum length. ax25 already has a min length
check through its protocol-specific validate callback.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/27/17 14:29), Willem de Bruijn wrote:
> 
> As your patch state, the contract is that any packet delivered to a
> driver has the entire L2 in its linear section. Drivers are not required
> to be robust against shorter packets, so there is no reason to test
> those.
> 
> One option is to limit your fix to known fixed-header protocols.
> In these cases hard_header_len is the minimum, so anything
> smaller must be dropped.

yes, but how would you you know that this is a fixed-header protocol
or a var-hdrlen protocol? AIUI the hard_header_len itself will not
tell you this info: it will be 77 for ax25, 14 for ethernet, 
but that does not tell me that ax25 is the "robust-er" driver
with a min requirement of 21 for the hdrlen.

That's why I was thinking of a IFF_L2_VARHDRLEN in the priv_flags
of the net_device.

> For protocols with variable header length it is fine to send packets
> shorter than hard_header_len, even with corrupted content (i.e.,
> even if they would fail that protocol's validate callback), as long as
> they exceed the minimum length. ax25 already has a min length
> check through its protocol-specific validate callback.

Another option that comes to mind.. the real thorn-in-the-flesh
here is the CAP_SYS_RAWIO check. Would it be a better idea to ask 
the test-suites (since they seem to be the major consumer of
that path) to use a special PF_PACKET socket option instead, that 
indicates "I'm testing robustness of the header, so let this one
slip past dev_validate_header at all times"?

It would mean the test suites would have to change slightly.

--Sowmini

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

On Fri, Jan 27, 2017 at 3:06 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (01/27/17 14:29), Willem de Bruijn wrote:
>>
>> As your patch state, the contract is that any packet delivered to a
>> driver has the entire L2 in its linear section. Drivers are not required
>> to be robust against shorter packets, so there is no reason to test
>> those.
>>
>> One option is to limit your fix to known fixed-header protocols.
>> In these cases hard_header_len is the minimum, so anything
>> smaller must be dropped.
>
> yes, but how would you you know that this is a fixed-header protocol
> or a var-hdrlen protocol? AIUI the hard_header_len itself will not
> tell you this info: it will be 77 for ax25, 14 for ethernet,
> but that does not tell me that ax25 is the "robust-er" driver
> with a min requirement of 21 for the hdrlen.

Right. Identifying the outliers is the hard part.

> That's why I was thinking of a IFF_L2_VARHDRLEN in the priv_flags
> of the net_device.
>
>> For protocols with variable header length it is fine to send packets
>> shorter than hard_header_len, even with corrupted content (i.e.,
>> even if they would fail that protocol's validate callback), as long as
>> they exceed the minimum length. ax25 already has a min length
>> check through its protocol-specific validate callback.
>
> Another option that comes to mind.. the real thorn-in-the-flesh
> here is the CAP_SYS_RAWIO check. Would it be a better idea to ask
> the test-suites (since they seem to be the major consumer of
> that path) to use a special PF_PACKET socket option instead, that

Introducing a sysctl has the same effect. It is not possible to
identify all callers dependent on the current ABI.

I see these options
- make capable() check conditional on sysctl (or interface flag, ..)
- limit capable() check to drivers with with .validate callback
- hardcode a list of known fixed length protocols that must fail
- let privileged applications shoot themselves in the foot (change nothing).

The first will break tests. Though with a runtime fix: flip the flag.

The second will break variable length header protocols unless
you exhaustively search for all variable length protocols and add
validate callbacks first.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/27/17 15:51), Willem de Bruijn wrote:
    :
> - limit capable() check to drivers with with .validate callback  
(aka second option below)
    :
> - let privileged applications shoot themselves in the foot (change nothing).

> The second will break variable length header protocols unless
> you exhaustively search for all variable length protocols and add
> validate callbacks first.

other than ax25, are there variable length header protocols out there
without ->validate, and which need the CAP_RAW_SYSIO branch?

I realize that, to an extent, even ethernet is a protocol whose
header is > 14 with vlan, but from the google search, seems like it
was mostly ax25 that really triggered a large part of the check.

If we think that there are a large number of these (that dont have a 
->validate, to fix up things as desired) I'd just go for the "change
nothing in pf_packet" option.

As I found out many drivers like ixgbe and sunvnet have defensive checks
in the Tx path anyway, and xen_netfront can also join that group with
a few simple checks.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

On Fri, Jan 27, 2017 at 4:58 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (01/27/17 15:51), Willem de Bruijn wrote:
>     :
>> - limit capable() check to drivers with with .validate callback
> (aka second option below)
>     :
>> - let privileged applications shoot themselves in the foot (change nothing).
>
>> The second will break variable length header protocols unless
>> you exhaustively search for all variable length protocols and add
>> validate callbacks first.
>
> other than ax25, are there variable length header protocols out there
> without ->validate, and which need the CAP_RAW_SYSIO branch?

I don't know. An exhaustive search of protocols (by header_ops) may be
needed to say for sure.

If there are none, then the solution indeed is quite simple.

> I realize that, to an extent, even ethernet is a protocol whose
> header is > 14 with vlan, but from the google search, seems like it
> was mostly ax25 that really triggered a large part of the check.
>
> If we think that there are a large number of these (that dont have a
> ->validate, to fix up things as desired) I'd just go for the "change
> nothing in pf_packet" option.
>
> As I found out many drivers like ixgbe and sunvnet have defensive checks
> in the Tx path anyway, and xen_netfront can also join that group with
> a few simple checks.

Okay. I suspect that there are few, if any. But this is fragile code.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Sowmini Varadhan]

On (01/27/17 19:19), Willem de Bruijn wrote:
> > other than ax25, are there variable length header protocols out there
> > without ->validate, and which need the CAP_RAW_SYSIO branch?
> 
> I don't know. An exhaustive search of protocols (by header_ops) may be
> needed to say for sure.
> 
> If there are none, then the solution indeed is quite simple.


I tried to start that exhaustive search, and it can be quite daunting:
if you are doing this by just code-inspection, it's easy to get
it wrong.. I havent quite given up yet, but it may be simpler to have
the drivers support some defensive code against bogus skb's in the
Tx path (the drivers will know, for sure, what's the min non-paged
len they need anyway).

--Sowmini

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Mon, 30 Jan 2017 11:26:03 -0500

> On (01/27/17 19:19), Willem de Bruijn wrote:
>> > other than ax25, are there variable length header protocols out there
>> > without ->validate, and which need the CAP_RAW_SYSIO branch?
>> 
>> I don't know. An exhaustive search of protocols (by header_ops) may be
>> needed to say for sure.
>> 
>> If there are none, then the solution indeed is quite simple.
> 
> 
> I tried to start that exhaustive search, and it can be quite daunting:
> if you are doing this by just code-inspection, it's easy to get
> it wrong.. I havent quite given up yet, but it may be simpler to have
> the drivers support some defensive code against bogus skb's in the
> Tx path (the drivers will know, for sure, what's the min non-paged
> len they need anyway).

I think it's easier to audit all the header_ops than to add defensive
code to 500+ drivers.

Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb_headlen() to the driver

[Willem de Bruijn]

On Mon, Jan 30, 2017 at 8:41 AM, David Miller <davem@davemloft.net> wrote:
> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Date: Mon, 30 Jan 2017 11:26:03 -0500
>
>> On (01/27/17 19:19), Willem de Bruijn wrote:
>>> > other than ax25, are there variable length header protocols out there
>>> > without ->validate, and which need the CAP_RAW_SYSIO branch?
>>>
>>> I don't know. An exhaustive search of protocols (by header_ops) may be
>>> needed to say for sure.
>>>
>>> If there are none, then the solution indeed is quite simple.
>>
>>
>> I tried to start that exhaustive search, and it can be quite daunting:
>> if you are doing this by just code-inspection, it's easy to get
>> it wrong.. I havent quite given up yet, but it may be simpler to have
>> the drivers support some defensive code against bogus skb's in the
>> Tx path (the drivers will know, for sure, what's the min non-paged
>> len they need anyway).
>
> I think it's easier to audit all the header_ops than to add defensive
> code to 500+ drivers.

This issue came up again in a slightly different context. I scanned
the implementations of header_ops. Variable length link layer headers
are quite common. Not necessarily as malleable as ax25, but in
the form of fixed headers with a limited set of optional extensions,
such as ipgre. For this reason, adding ->validate implementations for
each is infeasible, especially for a patch to net.

I think that the right approach is to finally introduce an explicit
dev->min_header_length field, and initialize that at least for
Ethernet and loopback.

I will send that and a related patch.