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: <20201020151913.3bfc8edb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 20 Oct 2020 15:19:13 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lijun Pan <ljp@...ux.vnet.ibm.com>
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 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;
	}

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.

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.