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]

Message-Id: <8AEE4003-FA15-4D69-9355-F15E1B737C0F@linux.vnet.ibm.com>
Date:   Tue, 20 Oct 2020 17:07:38 -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 20, 2020, at 4:33 PM, Jakub Kicinski <kuba@...nel.org> wrote:
> 
> On Tue, 20 Oct 2020 16:18:04 -0500 Lijun Pan wrote:
>>> 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]);
> 
> Please read my reply carefully.
> 
> What's the call path that leads to the address being wrong? If you set
> the address via ifconfig it will call ibmvnic_set_mac() of the driver.
> ibmvnic_set_mac() does the copy.
> 
> But it doesn't validate the address, which it should.

Sorry about that. The mac addr validation is performed on vios side when it receives the
change request. That’s why there is no mac validation code in vnic driver. 
In handle_change_mac_rsp(), &crq->change_mac_addr_rsp.mac_addr[0]
contains the current valid mac address, which may be different than the requested one, 
that is &crq->change_mac_addr.mac_addr[0].
crq->change_mac_addr.mac_addr is the requested one.
crq->change_mac_addr_rsp.mac_addr is the returned valid one.

Hope the above answers your doubt.