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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Oct 2020 00:34:25 +0200 From: Maciej Fijalkowski <maciej.fijalkowski@...el.com> To: Lijun Pan <ljp@...ux.ibm.com> Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org> Subject: Re: [PATCH net v2] ibmvnic: fix ibmvnic_set_mac On Wed, Oct 21, 2020 at 01:07:12AM -0500, Lijun Pan wrote: > Jakub Kicinski brought up a concern in ibmvnic_set_mac(). > 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. > > The fix is to add the handling for "adapter->state == VNIC_PROBED" case, > which saves the old mac address back to adapter->mac_addr, and > returns an error code. > > Fixes: 62740e97881c ("net/ibmvnic: Update MAC address settings after adapter reset") > Cc: Jakub Kicinski <kuba@...nel.org> > Signed-off-by: Lijun Pan <ljp@...ux.ibm.com> > --- > v2: change the subject from v1's > "ibmvnic: no need to update adapter->mac_addr before it completes" > handle adapter->state==VNIC_PROBED case in else statement. > > drivers/net/ethernet/ibm/ibmvnic.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index 4dd3625a4fbc..0d78e1e3d44c 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -1829,8 +1829,12 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p) > > rc = 0; > ether_addr_copy(adapter->mac_addr, addr->sa_data); > - if (adapter->state != VNIC_PROBED) > + if (adapter->state != VNIC_PROBED) { > rc = __ibmvnic_set_mac(netdev, addr->sa_data); > + } else { > + ether_addr_copy(adapter->mac_addr, netdev->dev_addr); > + rc = -EIO; Why suddenly you want to change the behavior for case when ibmvnic_set_mac is called for VNIC_PROBED state? I went through the previous discussion and I have a feeling that Jakub meant to simply call the is_valid_ether_addr() on addr->sa_data before the first ether_addr_copy and then act accordingly based on the validity of user supplied mac addr. And instead of yet another write to adapter->mac_addr that you're introducing you could just move the first ether_addr_copy (if addr->sa_data is valid) onto the if (adapter->state != VNIC_PROBED) condition. Right? > + } > > return rc; > } > -- > 2.23.0 >
Powered by blists - more mailing lists