[PATCH] fix MAC-setting for r8723au

[PATCH] fix MAC-setting for r8723au

[Dan Lenski]

Hi all,
The r8723au driver silently leaves the MAC address unchanged
when attempting to set it.

This was mentioned in a Debian security bug for macchanger
(https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=774898;msg=46) and
more recently has been causing me frustration with captive portals at
that give a short window of free connectivity, and refuse further
connections from the same MAC address.

This patch:

* Correctly sets the dev_addr field of struct net_device. The code to
  do this was for some reason commented out in the original version of
  the driver from Realtek:
  http://github.com/lwfinger/rtl8723au/blob/bc13943b872dc666a3cfa55407e7f9965f0aab52/os_dep/os_intfs.c#L827

* Removes the restriction wherein the MAC address cannot be set after
  the network driver is first brought up, and replaces it with a
  warning.  Because rtw_adapter->bup is set to 1 when the device is
  first opened, and _never_ set back to 0, it would be necessary to
  reload the module to change the MAC with this restriction.

I have tested repeatedly changing the MAC address while the device is
up, and there seem to be no ill effects, other than userspace tools
getting confused.

Thanks,
Dan


Dan Lenski (1):
  enable setting MAC address for r8723au

 drivers/staging/rtl8723au/os_dep/os_intfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.5.0

[PATCH] enable setting MAC address for r8723au

[Dan Lenski]

Signed-off-by: Dan Lenski <dlenski@gmail.com>
---
 drivers/staging/rtl8723au/os_dep/os_intfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
index b8848c2..228e19f 100644
--- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
+++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
@@ -240,8 +240,11 @@ static int rtw_net_set_mac_address(struct net_device *pnetdev, void *p)
 	struct rtw_adapter *padapter = netdev_priv(pnetdev);
 	struct sockaddr *addr = p;
 
-	if (!padapter->bup)
-		ether_addr_copy(padapter->eeprompriv.mac_addr, addr->sa_data);
+	if (padapter->bup)
+		DBG_8723A_LEVEL(_drv_warning_, "Trying to set MAC address while bup =%d\n", padapter->bup);
+	ether_addr_copy(padapter->eeprompriv.mac_addr, addr->sa_data);
+	ether_addr_copy(pnetdev->dev_addr, addr->sa_data);
+
 	return 0;
 }
 
-- 
2.5.0

Re: [PATCH] enable setting MAC address for r8723au

[Larry Finger]

On 12/20/2015 08:28 PM, Dan Lenski wrote:
> Signed-off-by: Dan Lenski <dlenski@gmail.com>

The commit message should be in this patch rather than in the non-patch previous 
mail. If this patch were to be accepted, all that explanation would be lost!

Rather than issuing a warning when the MAC is changed after the interface has 
been brought up, have you considered changing the value of rtw_adapter->bup to 
zero whenever the connection goes down? Would that help with the confusion in 
the user-space tools?

NACK.


Larry

> ---
>   drivers/staging/rtl8723au/os_dep/os_intfs.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
> index b8848c2..228e19f 100644
> --- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
> +++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
> @@ -240,8 +240,11 @@ static int rtw_net_set_mac_address(struct net_device *pnetdev, void *p)
>   	struct rtw_adapter *padapter = netdev_priv(pnetdev);
>   	struct sockaddr *addr = p;
>
> -	if (!padapter->bup)
> -		ether_addr_copy(padapter->eeprompriv.mac_addr, addr->sa_data);
> +	if (padapter->bup)
> +		DBG_8723A_LEVEL(_drv_warning_, "Trying to set MAC address while bup =%d\n", padapter->bup);
> +	ether_addr_copy(padapter->eeprompriv.mac_addr, addr->sa_data);
> +	ether_addr_copy(pnetdev->dev_addr, addr->sa_data);
> +
>   	return 0;
>   }
>
>

Re: [PATCH] enable setting MAC address for r8723au

[Daniel Lenski]

On Mon, Dec 21, 2015 at 9:53 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 12/20/2015 08:28 PM, Dan Lenski wrote:
>>
>> Signed-off-by: Dan Lenski <dlenski@gmail.com>
>
>
> The commit message should be in this patch rather than in the non-patch
> previous mail. If this patch were to be accepted, all that explanation would
> be lost!
>
> Rather than issuing a warning when the MAC is changed after the interface
> has been brought up, have you considered changing the value of
> rtw_adapter->bup to zero whenever the connection goes down? Would that help
> with the confusion in the user-space tools?

No. rtw_adapter isn't visible to userspace at all. NetworkManager, for
instance, seems to get confused when *any* up interface changes its
MAC address.

bup should *not* be reset to zero when the device is closed.
netdev_open23a() checks or bup==0 and calls rtl8723au_hal_init() to do
hw initialization and firmware download if so. This is unnecessary
after subsequent re-opening, which is why netdev_close() doesn't set
bup=0.

> NACK.

I'll resubmit with the commit message fixed and the warning removed.

Dan

Re: [PATCH] enable setting MAC address for r8723au

[Jes Sorensen]

Daniel Lenski <dlenski@gmail.com> writes:
> On Mon, Dec 21, 2015 at 9:53 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
>> On 12/20/2015 08:28 PM, Dan Lenski wrote:
>>>
>>> Signed-off-by: Dan Lenski <dlenski@gmail.com>
>>
>>
>> The commit message should be in this patch rather than in the non-patch
>> previous mail. If this patch were to be accepted, all that explanation would
>> be lost!
>>
>> Rather than issuing a warning when the MAC is changed after the interface
>> has been brought up, have you considered changing the value of
>> rtw_adapter->bup to zero whenever the connection goes down? Would that help
>> with the confusion in the user-space tools?
>
> No. rtw_adapter isn't visible to userspace at all. NetworkManager, for
> instance, seems to get confused when *any* up interface changes its
> MAC address.
>
> bup should *not* be reset to zero when the device is closed.
> netdev_open23a() checks or bup==0 and calls rtl8723au_hal_init() to do
> hw initialization and firmware download if so. This is unnecessary
> after subsequent re-opening, which is why netdev_close() doesn't set
> bup=0.
>
>> NACK.
>
> I'll resubmit with the commit message fixed and the warning removed.
>

In addition, do *not* overwrite the eeprompriv.mac_addr - that struct is
a clean copy of the eeprom's data and should not be modified.

Please changed the dev entry and make sure they driver updates from
there instead.

Second, please CC me directly as the driver maintainer.

For longer term, please try out rtl8xxxu, hopefully we can
rm -rf drivers/staging/rtl8723au soon.

Jes

Re: [PATCH] enable setting MAC address for r8723au

[Daniel Lenski]

On Wed, Dec 23, 2015 at 3:18 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> Daniel Lenski <dlenski@gmail.com> writes:
>> On Mon, Dec 21, 2015 at 9:53 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
>>> On 12/20/2015 08:28 PM, Dan Lenski wrote:
>>>>
>>>> Signed-off-by: Dan Lenski <dlenski@gmail.com>
>>>
>>>
>>> The commit message should be in this patch rather than in the non-patch
>>> previous mail. If this patch were to be accepted, all that explanation would
>>> be lost!
>>>
>>> Rather than issuing a warning when the MAC is changed after the interface
>>> has been brought up, have you considered changing the value of
>>> rtw_adapter->bup to zero whenever the connection goes down? Would that help
>>> with the confusion in the user-space tools?
>>
>> No. rtw_adapter isn't visible to userspace at all. NetworkManager, for
>> instance, seems to get confused when *any* up interface changes its
>> MAC address.
>>
>> bup should *not* be reset to zero when the device is closed.
>> netdev_open23a() checks or bup==0 and calls rtl8723au_hal_init() to do
>> hw initialization and firmware download if so. This is unnecessary
>> after subsequent re-opening, which is why netdev_close() doesn't set
>> bup=0.
>>
>>> NACK.
>>
>> I'll resubmit with the commit message fixed and the warning removed.
>>
>
> In addition, do *not* overwrite the eeprompriv.mac_addr - that struct is
> a clean copy of the eeprom's data and should not be modified.
>
> Please changed the dev entry and make sure they driver updates from
> there instead.

I left that part alone (overwriting the eeprompriv.mac_addr) because
the existing code relies on it containing the correct *current* MAC
address in numerous places. But, fair enough.

This will require a much more complex patch, because there are
numerous functions which assume that eeprompriv.mac_addr is the
current address. (And some of these functions only receive the struct
rtw_adapter as an argument, rather than the complete netdev.)

> Second, please CC me directly as the driver maintainer.
>
> For longer term, please try out rtl8xxxu, hopefully we can
> rm -rf drivers/staging/rtl8723au soon.

Woah, I didn't know that driver existed. I will take a look.

Thanks,
Dan

Re: [PATCH] enable setting MAC address for r8723au

[Jes Sorensen]

Daniel Lenski <dlenski@gmail.com> writes:
> On Wed, Dec 23, 2015 at 3:18 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>> Daniel Lenski <dlenski@gmail.com> writes:
>> In addition, do *not* overwrite the eeprompriv.mac_addr - that struct is
>> a clean copy of the eeprom's data and should not be modified.
>>
>> Please changed the dev entry and make sure they driver updates from
>> there instead.
>
> I left that part alone (overwriting the eeprompriv.mac_addr) because
> the existing code relies on it containing the correct *current* MAC
> address in numerous places. But, fair enough.
>
> This will require a much more complex patch, because there are
> numerous functions which assume that eeprompriv.mac_addr is the
> current address. (And some of these functions only receive the struct
> rtw_adapter as an argument, rather than the complete netdev.)
>
>> Second, please CC me directly as the driver maintainer.
>>
>> For longer term, please try out rtl8xxxu, hopefully we can
>> rm -rf drivers/staging/rtl8723au soon.
>
> Woah, I didn't know that driver existed. I will take a look.

It's pretty new and should be available in 4.4. I'm still working on
it, so there may be a few rough edges.

Cheers,
Jes