netdev - Re: [PATCH net] ibmvnic: save changed mac address to adapter->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]

Date:   Tue, 20 Oct 2020 16:18:04 -0500
From:   Lijun Pan <ljp@...ux.vnet.ibm.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Lijun Pan <ljp@...ux.ibm.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] ibmvnic: save changed mac address to
 adapter->mac_addr



> On Oct 19, 2020, at 7:11 PM, Jakub Kicinski <kuba@...nel.org> wrote:
> 
> On Thu, 15 Oct 2020 23:57:15 -0500 Lijun Pan wrote:
>> After mac address change request completes successfully, the new mac
>> address need to be saved to adapter->mac_addr as well as
>> netdev->dev_addr. Otherwise, adapter->mac_addr still holds old
>> data.
> 
> Do you observe this in practice? Can you show us which path in
> particular you see this happen on?
> 
> AFAICS ibmvnic_set_mac() copies the new MAC addr into adapter->mac_addr
> before making a request.
> 
> If anything is wrong here is that it does so regardless if MAC addr 
> is valid.
> 

Yes, I ran some internal test to check the mac address in adapter->mac_addr, and
it is the old data. If you run ifconfig command to change mac addr, the netdev->dev_addr
is changed afterwards, and if you run ifocnfig again, it will show the new mac addr. However,
since we did not check adapter->mac_addr in this use case, this bug was not exposed.

This vnic driver is little bit different than other physical NIC driver. All the control paths
are negotaited with VIOS server, and data paths are through DMA mapping.

__ibmvnic_set_mac copies the new mac addr to crq by
	ether_addr_copy(&crq.change_mac_addr.mac_addr[0], dev_addr);
and then send the change request by
	rc = ibmvnic_send_crq(adapter, &crq);
Now adapter->mac_addr still has the old data.

When the request is handled by VIOS server, an interrupt is triggered, and 
handle_change_mac_rsp is called. 
Now it is time to copy the new mac to netdev->dev_addr, and adatper->mac_addr.
	ether_addr_copy(netdev->dev_addr,
			&crq->change_mac_addr_rsp.mac_addr[0]);
It missed the copy for adapter->mac_addr, which is what I add in this patch.
+	ether_addr_copy(adapter->mac_addr,
+			&crq->change_mac_addr_rsp.mac_addr[0]);

>> Fixes: 62740e97881c("net/ibmvnic: Update MAC address settings after adapter reset")
>                     ^
>                      missing space here 
> 

Will fix this in v2.

Lijun