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

Date:   Tue, 20 Oct 2020 17:46:42 -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 5:19 PM, Jakub Kicinski <kuba@...nel.org> wrote:
> 
> On Tue, 20 Oct 2020 17:07:38 -0500 Lijun Pan wrote:
>>> 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. 
> 
> The problem is that there is validation in the driver:
> 
> static int __ibmvnic_set_mac(struct net_device *netdev, u8 *dev_addr)
> {
> 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> 	union ibmvnic_crq crq;
> 	int rc;
> 
> 	if (!is_valid_ether_addr(dev_addr)) {
> 		rc = -EADDRNOTAVAIL;
> 		goto err;
> 	}
> 

I think this one validates whether the address is of a legal format.
The validation in VIOS checks it according to the Address Control List
entries set up by the administrator.

> And ibmvnic_set_mac() does this:
> 
> 	ether_addr_copy(adapter->mac_addr, addr->sa_data);
> 	if (adapter->state != VNIC_PROBED)
> 		rc = __ibmvnic_set_mac(netdev, addr->sa_data);
> 
> So if state == VNIC_PROBED, the user can assign an invalid address to
> adapter->mac_addr, and ibmvnic_set_mac() will still return 0.

Let me address this problem, and send a separate patch.

> 
> That's a separate issue, for a separate patch, though, so you can send 
> a v2 of this patch regardless.
> 
>> 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.
> 
> Oh! The address in reply can be different than the requested one?
> Please add a comment stating that in the code.

I have sent v2 with comment in the code.