netdev - Re: hard_header_len vs needed

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, 9 Jun 2014 11:16:55 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Cong Wang <cwang@...pensource.com>
Cc:	David Miller <davem@...emloft.net>,
	Timo Teräs <timo.teras@....fi>,
	netdev <netdev@...r.kernel.org>
Subject: Re: hard_header_len vs needed_headroom

On Thu, Jun 5, 2014 at 4:39 PM, Cong Wang <cwang@...pensource.com> wrote:
> Hello,
>
>
> While debugging a skb header push crash, I noticed that
> new ip tunnel code uses dev->needed_headroom as the
> length of hardware header:
>
> dev->needed_headroom = t_hlen + hlen;
>
> in IPIP tunnel case it's the ethernet header length plus IPv4
> header length. And it leaves dev->hard_header_len as 0.
>
> But there are some places assuming
> dev->hard_header_len is the length of L2 header, at least
> tcf_mirred() calls skb_push(skb, skb->dev->hard_header_len)
> to reset skb->data back to L2 header. Current code is broken.
>
> I think ip tunnel code is wrong, according to LL_RESERVED_SPACE(),
> it seems dev->hard_header_len should be the L2 header while
> dev->needed_headroom should be the *additional* headroom needed
> by hardware. That also means skb_push(dev->hard_header_len) is
> correct.
>
> Therefore, I think IP tunnel should set its dev->hard_header_len to
> lower_dev->hard_header_len + tunnel->hlen + sizeof(struct iphdr),
> and set its dev->needed_headroom to lower_dev->needed_headroom.
> See the attached patch for details.
>
> But gretap seems to be an exception, it calls ether_setup() to init its
> dev->hard_header_len to ETH_HLEN. My patch probably breaks it,
> I think it should set its tunnel->hlen = gre_hdr_len + ETH_HLEN.
>

When ip-tunnel was written I thought LL_RESERVED_SPACE() is used to
reserve header space. Obviously I missed tcf_mirred()
At this point I think easiest fix would be using LL_RESERVED_SPACE()
in tcf_mirred().
Later we can change ip-tunnel code to use hard_header and
needed_headroom as you have menioned.

Thanks,
Pravin.


> What do you think?
>
>
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html