netdev - Re: [PATCH RFC net-next] packet: always ensure that we pass hard_header_len bytes in skb

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]

Message-ID: <CAF=yD-L66dFV64fx21A3059LRiot_-j0Ykp9GkRFwf52099upQ@mail.gmail.com>
Date:   Thu, 26 Jan 2017 19:08:44 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC net-next] packet: always ensure that we pass
 hard_header_len bytes in skb_headlen() to the driver

On Thu, Jan 26, 2017 at 4:37 PM, Sowmini Varadhan
<sowmini.varadhan@...cle.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.
>