netdev - Re: [PATCH net] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet

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: <290a8e03-1d24-a84f-751c-6fc27f04bba0@gmail.com>
Date:   Tue, 28 May 2019 14:02:33 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Davide Caratti <dcaratti@...hat.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Cc:     shuali@...hat.com, Eli Britstein <elibr@...lanox.com>
Subject: Re: [PATCH net] net/sched: act_pedit: fix 'ex munge' on network
 header in case of QinQ packet



On 5/28/19 1:50 PM, Davide Caratti wrote:
> Like it has been done in commit 2ecba2d1e45b ("net: sched: act_csum: Fix
> csum calc for tagged packets"), also 'pedit' needs to adjust the network
> offset when multiple tags are present in the packets: otherwise wrong IP
> headers (but good checksums) can be observed with the following command:

...

> +again:
> +		switch (protocol) {
> +		case cpu_to_be16(ETH_P_8021AD): /* fall through */
> +		case cpu_to_be16(ETH_P_8021Q):
> +			if (skb_vlan_tag_present(skb) &&
> +			    !orig_vlan_tag_present) {
> +				protocol = skb->protocol;
> +				orig_vlan_tag_present = true;
> +			} else {
> +				struct vlan_hdr *vlan;
> +
> +				vlan = (struct vlan_hdr *)skb->data;
> +				protocol = vlan->h_vlan_encapsulated_proto;
> +				skb_pull(skb, VLAN_HLEN);
> +				skb_reset_network_header(skb);
> +				(*vlan_hdr_count)++;
> +			}
> +			goto again;

What prevents this loop to access data not yet in skb->head ?

skb_header_pointer() (or pskb_may_pull()) seems needed.