netdev - Re: [net] net/packet: fix incoming receive for L3 devices without visible hard header

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] [day] [month] [year] [list]

Message-ID: <CA+FuTSe5jHJhE=a1uOYi8+Le2-6Naea2nY8iji2GxxjdEEr69Q@mail.gmail.com>
Date:   Fri, 20 Nov 2020 21:23:08 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Network Development <netdev@...r.kernel.org>,
        Xie He <xie.he.0141@...il.com>
Subject: Re: [net] net/packet: fix incoming receive for L3 devices without
 visible hard header

On Thu, Nov 19, 2020 at 10:05 PM Eyal Birger <eyal.birger@...il.com> wrote:
>
> In the patchset merged by commit b9fcf0a0d826
> ("Merge branch 'support-AF_PACKET-for-layer-3-devices'") L3 devices which
> did not have header_ops were given one for the purpose of protocol parsing
> on af_packet transmit path.
>
> That change made af_packet receive path regard these devices as having a
> visible L3 header and therefore aligned incoming skb->data to point to the
> skb's mac_header. Some devices, such as ipip, xfrmi, and others, do not
> reset their mac_header prior to ingress and therefore their incoming
> packets became malformed.
>
> Ideally these devices would reset their mac headers, or af_packet would be
> able to rely on dev->hard_header_len being 0 for such cases, but it seems
> this is not the case.
>
> Fix by changing af_packet RX ll visibility criteria to include the
> existence of a '.create()' header operation, which is used when creating
> a device hard header - via dev_hard_header() - by upper layers, and does
> not exist in these L3 devices.
>
> Fixes: b9fcf0a0d826 ("Merge branch 'support-AF_PACKET-for-layer-3-devices'")
> Signed-off-by: Eyal Birger <eyal.birger@...il.com>

Thanks for the fix. This makes sense to me.

Recent discussions on this point also agreed that whether or not
headers are exposed to code above the device driver depends
on dev->hard_header_len and dev->header_ops->create (and
the two have to be consistent with one another).

dev->header_ops->parse_protocol is a best effort approach to
infer a protocol in cases where the caller did not specify it. But
as best effort, its existence or absence does not define the
device header, so testing only header_ops != NULL is insufficient.

> ---
>  net/packet/af_packet.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index cefbd50c1090..a241059fd536 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -93,8 +93,8 @@
>
>  /*
>     Assumptions:
> -   - If the device has no dev->header_ops, there is no LL header visible
> -     above the device. In this case, its hard_header_len should be 0.
> +   - If the device has no dev->header_ops->create, there is no LL header
> +     visible above the device. In this case, its hard_header_len should be 0.
>       The device may prepend its own header internally. In this case, its
>       needed_headroom should be set to the space needed for it to add its
>       internal header.
> @@ -108,21 +108,21 @@
>  On receive:
>  -----------
>
> -Incoming, dev->header_ops != NULL
> +Incoming, dev->header_ops != NULL && dev->header_ops->create != NULL
>     mac_header -> ll header
>     data       -> data
>
> -Outgoing, dev->header_ops != NULL
> +Outgoing, dev->header_ops != NULL && dev->header_ops->create != NULL
>     mac_header -> ll header
>     data       -> ll header
>
> -Incoming, dev->header_ops == NULL
> +Incoming, dev->header_ops == NULL || dev->header_ops->create == NULL
>     mac_header -> data
>       However drivers often make it point to the ll header.
>       This is incorrect because the ll header should be invisible to us.
>     data       -> data
>
> -Outgoing, dev->header_ops == NULL
> +Outgoing, dev->header_ops == NULL || dev->header_ops->create == NULL
>     mac_header -> data. ll header is invisible to us.
>     data       -> data
>
> @@ -272,6 +272,18 @@ static bool packet_use_direct_xmit(const struct packet_sock *po)
>         return po->xmit == packet_direct_xmit;
>  }
>
> +static bool packet_ll_header_rcv_visible(const struct net_device *dev)
> +{
> +       /* The device has an explicit notion of ll header,
> +        * exported to higher levels
> +        *
> +        * Otherwise, the device hides details of its frame
> +        * structure, so that corresponding packet head is
> +        * never delivered to user.
> +        */
> +       return dev->header_ops && dev->header_ops->create;
> +}
> +

Perhaps a dev_has_header(..) in include/linux/netdevice.h

And then the same in the Incoming/Outgoing comments above.