Date:   Mon, 7 Sep 2020 11:05:31 +0200
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Xie He <xie.he.0141@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: Question about dev_validate_header used in af_packet.c

On Sun, Sep 6, 2020 at 1:21 AM Xie He <xie.he.0141@...il.com> wrote:
>
> On Sat, Sep 5, 2020 at 3:24 PM Xie He <xie.he.0141@...il.com> wrote:
> >
> > Hi Willem,
> >
> > I have a question about the function dev_validate_header used in
> > af_packet.c. Can you help me? Thanks!
> >
> > I see when the length of the data is smaller than hard_header_len, and
> > when the user is "capable" enough, the function will accept it and pad
> > it with 0s, without validating the header with header_ops->validate.
> >
> > But I think if the driver is able to accept variable-length LL
> > headers, shouldn't we just pass the data to header_ops->validate and
> > let it check the header's validity, and then just pass the validated
> > data to the driver for transmission?
> >
> > Why when the user is "capable" enough, can it bypass the
> > header_ops->validate check? And why do we need to pad the data with
> > 0s? Won't this make the driver confused about the real length of the
> > data?
>
> Oh. I just realized that the padding of zeros won't actually make the
> data longer. The padded zeros are not part of the data so the length
> of the data is kept unchanged. The padding is probably because some
> weird drivers are expecting this. (What drivers are them? Can we fix
> them?)
>
> I can also understand now the ability of a "capable" user to bypass
> the header_ops->validate check. It is probably for testing purposes.
> (Does this mean the root user will always bypass this check?)

Apologies for the delay.

The commit that introduced the code probably summarizes state better
than I would write off the cuff:

"
commit 2793a23aacbd754dbbb5cb75093deb7e4103bace
Author: Willem de Bruijn <willemb@...gle.com>
Date:   Wed Mar 9 21:58:32 2016 -0500

    net: validate variable length ll headers

    Netdevice parameter hard_header_len is variously interpreted both as
    an upper and lower bound on link layer header length. The field is
    used as upper bound when reserving room at allocation, as lower bound
    when validating user input in PF_PACKET.

    Clarify the definition to be maximum header length. For validation
    of untrusted headers, add an optional validate member to header_ops.

    Allow bypassing of validation by passing CAP_SYS_RAWIO, for instance
    for deliberate testing of corrupt input. In this case, pad trailing
    bytes, as some device drivers expect completely initialized headers.

    See also http://comments.gmane.org/gmane.linux.network/401064

    Signed-off-by: Willem de Bruijn <willemb@...gle.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>
"

The CAP_SYS_RAWIO exception indeed was requested to be able to
purposely test devices against bad inputs. The gmane link
unfortunately no longer works, but this was the discussion thread:
https://www.mail-archive.com/netdev@vger.kernel.org/msg99920.html

It zeroes the packet up max_header_len to ensure that an unintentional
short packet will at least not result in reading undefined data. Now
that the dust has settled around the min_header_len/max_header_len
changes, maybe now is a good time to revisit removing that
CAP_SYS_RAWIO loophole.

Powered by blists - more mailing lists