netdev - Re: [PATCH net-next] net/vxlan: Avoid unaligned access in vxlan_build

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:   Tue, 20 Sep 2016 12:31:08 -0400
From:   Sowmini Varadhan <sowmini.varadhan@...cle.com>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        hannes@...essinduktion.org, aduyck@...antis.com,
        daniel@...earbox.net, pabeni@...hat.com
Subject: Re: [PATCH net-next] net/vxlan: Avoid unaligned access in
 vxlan_build_skb()

On (09/20/16 18:11), Jiri Benc wrote:
> > The vxlan header is at offset (14 + 20 + 8) into the packet,
> > so the vxh is not aligned in vxlan_build_skb. Use [get/put]_unaligned
> > functions to modify flags and vni field in the vxh.
> 
> How did you calculate that? IP header should be aligned to 4 bytes, UDP
> header is 8 bytes, thus VXLAN header is also aligned to 4 bytes.

The vxlan header is after the ethernet header (14 bytes),
IP header (20 bytes, assuming no options) and udp header (8 bytes).
Post the skb_reserve adjustments (see computations in in mld_newpack(),
for example), this triggers an unaligned access on sparc.

> If you went this way, it would be better to make two local variables
> for vx_flags and vx_vni, store to them and do a single put_unaligned
> after the condition. That way, you would have two less put_unaligned and
> no get_unaligned in the remote csum case.

Ok, that is certainly possible.

> And the code would be
> cleaner. And you're missing vx_flags being accessed in
> vxlan_build_gbp_hdr.

Sure, and there's also potential unaligned access in vxlan_build_gpe_hdr.

But as I was telling Tom, this problem is much deeper than this fix, and 
I dont have the facility, at the moment, to test out every one of these
code paths. We would have to fix these, one at a time, in subsequent
patches. This one just fixes the top-level basic code paths.

> But I think this is not needed at all, see above.
> 
>  Jiri