netdev - RE: [PATCH net-next v2 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]

Message-ID: <AM6PR05MB5879232068A16CF20F9E450ED17F0@AM6PR05MB5879.eurprd05.prod.outlook.com>
Date:   Fri, 22 Feb 2019 12:30:15 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.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 net-next v2 1/7] net: Don't set transport offset to
 invalid value

> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> Sent: 21 February, 2019 19:28
> 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; Eran Ben Elisha <eranbe@...lanox.com>; Tariq Toukan
> <tariqt@...lanox.com>
> Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to
> invalid value
> 
> On Thu, Feb 21, 2019 at 7:40 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>
> 
> qdisc_pkt_len_init also expects skb_transport_header(skb) to always be
> set for gso packets.
> 
> Once net is merged into net-next, commit d5be7f632bad ("net: validate

This commit is already in net-next, isn't it?

> untrusted gso packets without csum offload") will ensure that packets
> that fail flow dissection do not make it into the stack. But we have
> to skip dissection in some cases, like tun [1].

OK, got you. However, is everything OK with patch [1]? It fixes false
positives, when a packet was dropped because network_header had not been
set yet for dissection to succeed, but what about evil packets that have
no network_offset at the moment of calling virtio_net_hdr_to_skb? Why
are all of them considered valid?

> I think we need to add a check in qdisc_pkt_len_init to skip the gso
> size estimation branch if !skb_transport_header_was_set(skb).
> 
> Otherwise this patch set looks good to me. To avoid resubmitting
> everything we can fix up the qdisc_pkt_len_init in a follow-up, in
> which case I'm happy to add my Acked-by to this series.

I'll add this check and submit the patch soon. Thanks for reviewing!

> [1] http://patchwork.ozlabs.org/patch/1044429/