netdev - Re: [PATCH v2 03/10] nl80211: Prefer ether_addr

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] [day] [month] [year] [list]

Message-ID: <1464690887.3076.24.camel@sipsolutions.net>
Date:	Tue, 31 May 2016 12:34:47 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Kirtika Ruchandani <kirtika.ruchandani@...il.com>,
	Julian Calaby <julian.calaby@...il.com>
Cc:	linux-wireless <linux-wireless@...r.kernel.org>,
	Tina Ruchandani <ruchandani.tina@...il.com>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 03/10] nl80211: Prefer ether_addr_copy


> The requirement is to be __aligned(2). I've added 4 instances of
> ether_addr_copy with 8 addresses as arguments.  Of these, the 4
> src arguments are really the same type (i.e. nla_data acting on a
> const nlattr*), so I'll try to reason about the 5 total cases below -
> 1. cfg->dst_mac should be 16-bit aligned due to the layout of
> struct cfg80211_wowlan_tcp. Its offset is 10 or 12 bytes in the
> structure depending on the system.

I wouldn't want to rely on that, since internal structures can be
changed pretty easily. If necessary, add __aligned(2) to the struct
members where appropriate.

> 2 and 3. For mac_addr and mac_addr_mask, nl80211_parse_random_mac
> takes these in as u8* (and hence does not guarantee alignment?)
> Both the callers of this function today pass in arguments that are
> explicitly __aligned(2). But this cannot be said of future potential
> callers
> - so perhaps my patch introduces a bug?

That should be documented, but it's also pretty tricky to
review/maintain that.

I general, I don't really see much reason to use ether_addr_copy() in
any of these code paths - it's not really a performance thing (in part
due to the alignment, ether_addr_copy can be faster)

> 4. Based on struct cfg80211_acl_data, acl->mac_addrs[i] should be not
> guaranteed to be __aligned(2).

See above.

> 5. For all the nla_data src arguments, the nla_data function returns
> ((char*) foo + 5) for pointer foo. So likely not __aligned(2).

Those should be aligned since the data is copied into an SKB, and +5
doesn't seem right - nla_data() should be +4.

johannes