netdev - Re: [PATCH net-next v2 1/3] net: add check for current MAC address in dev_set_mac

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: <18b2b4a1-60b8-164f-ea31-5744950e138d@intel.com>
Date: Tue, 20 Jun 2023 12:29:00 +0200
From: Piotr Gardocki <piotrx.gardocki@...el.com>
To: Gal Pressman <gal@...dia.com>, <netdev@...r.kernel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <przemyslaw.kitszel@...el.com>,
	<michal.swiatkowski@...ux.intel.com>, <pmenzel@...gen.mpg.de>,
	<kuba@...nel.org>, <maciej.fijalkowski@...el.com>,
	<anthony.l.nguyen@...el.com>, <simon.horman@...igine.com>,
	<aleksander.lobakin@...el.com>
Subject: Re: [PATCH net-next v2 1/3] net: add check for current MAC address in
 dev_set_mac_address

On 20.06.2023 09:16, Gal Pressman wrote:
> On 13/06/2023 15:24, Piotr Gardocki wrote:
>> In some cases it is possible for kernel to come with request
>> to change primary MAC address to the address that is already
>> set on the given interface.
>>
>> This patch adds proper check to return fast from the function
>> in these cases.
>>
>> An example of such case is adding an interface to bonding
>> channel in balance-alb mode:
>> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
>> ip link set bond0 up
>> ifenslave bond0 <eth>
>>
>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@...el.com>
> 
> Hello Piotr,
> 
> I believe this patch has an (unintended?) side effect.
> The early return in dev_set_mac_address() makes it so
> 'dev->addr_assign_type' doesn't get assigned with NET_ADDR_SET, I don't
> think this is the correct behavior.

Hi Gal,

I checked it, you're right. When the addr_assign_type is PERM or RANDOM
and user or some driver sets the same MAC address the type doesn't change
to NET_ADDR_SET. In my testing I didn't notice issues with that, but I'm
sure there are cases I didn't cover. Did you discover any useful cases
that broke after this patch or did you just notice it in code?

The less invasive solution might be to skip only call to ndo_set_mac_address
if the address doesn't change, but call everything else - I suppose the
notifying mechanism would be required if we change addr_assign_type, right?

The patch set was already in v3 and it's applied to netdev next queue.
I'll let maintainers decide how to proceed with it now. I can take care of it,
but need to know whether to submit new patch or send v4.
@Jakub Kicinski, could you please take a look at request and give us some guidance?