Message ID | 1485274309-201670-1-git-send-email-sowmini.varadhan@oracle.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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.
> 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.
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.
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. >
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
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
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.
> " 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.
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
> 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.
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
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.
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.
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.
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
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.
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.
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);
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(-)