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

[Sowmini Varadhan <sowmini.varadhan@oracle.com>]

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.