netdev - Re: [PATCH 1/7] net: Don't set transport offset to invalid value

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]
Date:   Mon, 14 Jan 2019 11:49:05 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Maxim Mikityanskiy <maximmi@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Jason Wang <jasowang@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value

On Mon, Jan 14, 2019 at 8:20 AM Maxim Mikityanskiy <maximmi@...lanox.com> wrote:
>
> If the socket was created with socket(AF_PACKET, SOCK_RAW, 0),
> skb->protocol will be unset, __skb_flow_dissect() will fail, and
> skb_probe_transport_header() will fall back to the offset_hint, making
> the resulting skb_transport_offset incorrect.
>
> If, however, there is no transport header in the packet,
> transport_header shouldn't be set to an arbitrary value.
>
> Fix it by leaving the transport offset unset if it couldn't be found, to
> be explicit rather than to fill it with some wrong value. It changes the
> behavior, but if some code relied on the old behavior, it would be
> broken anyway, as the old one is incorrect.
>
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>
> ---
>  drivers/net/tap.c                 |  4 ++--
>  drivers/net/tun.c                 |  4 ++--
>  drivers/net/xen-netback/netback.c | 19 +++++++++++--------
>  include/linux/skbuff.h            | 14 +++++++-------
>  net/packet/af_packet.c            |  6 +++---
>  5 files changed, 25 insertions(+), 22 deletions(-)

This is a lot of code change. This would do.

@@ -2434,8 +2434,6 @@ static inline void
skb_probe_transport_header(struct sk_buff *skb,

        if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
                skb_set_transport_header(skb, keys.control.thoff);
-       else
-               skb_set_transport_header(skb, offset_hint);
 }

Though leaving an unused argument is a bit ugly. For net-next, indeed
better to clean up (please mark your patchset with net or net-next,
btw)

>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 443b2694130c..a35b44b13a34 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -712,7 +712,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>                         goto err_kfree;
>         }
>
> -       skb_probe_transport_header(skb, ETH_HLEN);
> +       skb_try_probe_transport_header(skb);
>
>         /* Move network header to the right position for VLAN tagged packets */
>         if ((skb->protocol == htons(ETH_P_8021Q) ||
> @@ -1177,7 +1177,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
>                         goto err_kfree;
>         }
>
> -       skb_probe_transport_header(skb, ETH_HLEN);
> +       skb_try_probe_transport_header(skb);
>
>         /* Move network header to the right position for VLAN tagged packets */
>         if ((skb->protocol == htons(ETH_P_8021Q) ||
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index a4fdad475594..f73a156379e6 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1927,7 +1927,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>         }
>
>         skb_reset_network_header(skb);
> -       skb_probe_transport_header(skb, 0);
> +       skb_try_probe_transport_header(skb);
>
>         if (skb_xdp) {
>                 struct bpf_prog *xdp_prog;
> @@ -2480,7 +2480,7 @@ static int tun_xdp_one(struct tun_struct *tun,
>
>         skb->protocol = eth_type_trans(skb, tun->dev);
>         skb_reset_network_header(skb);
> -       skb_probe_transport_header(skb, 0);
> +       skb_try_probe_transport_header(skb);
>
>         if (skb_xdp) {
>                 err = do_xdp_generic(xdp_prog, skb);
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 80aae3a32c2a..b49b6e56ca47 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
>                 struct xen_netif_tx_request *txp;
>                 u16 pending_idx;
>                 unsigned data_len;
> +               bool th_set;
>
>                 pending_idx = XENVIF_TX_CB(skb)->pending_idx;
>                 txp = &queue->pending_tx_info[pending_idx].req;
> @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
>                         continue;
>                 }
>
> -               skb_probe_transport_header(skb, 0);
> +               th_set = skb_try_probe_transport_header(skb);

Can use skb_transport_header_was_set(). Then at least there is no need
to change the function's return value.

Normally, a GSO packet will also have skb_transport_header set even
from packet sockets, due to skb_partial_csum_set called from
virtio_net_hdr_to_skb. But I don't think we can be sure that TSO
packets without checksum offload are dropped before reaching this
code.