diff mbox series

[1/3,v3] net: usb: r8152: Check used MAC passthrough address

Message ID 20220105151427.8373-1-aaron.ma@canonical.com
State New
Headers show
Series [1/3,v3] net: usb: r8152: Check used MAC passthrough address | expand

Commit Message

Aaron Ma Jan. 5, 2022, 3:14 p.m. UTC
When plugin multiple r8152 ethernet dongles to Lenovo Docks
or USB hub, MAC passthrough address from BIOS should be
checked if it had been used to avoid using on other dongles.

Currently builtin r8152 on Dock still can't be identified.
First detected r8152 will use the MAC passthrough address.

v2:
Skip builtin PCI MAC address which is share MAC address with
passthrough MAC.
Check thunderbolt based ethernet.

v3:
Add return value.

Fixes: f77b83b5bbab ("net: usb: r8152: Add MAC passthrough support for
more Lenovo Docks")
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Andrew Lunn Jan. 5, 2022, 5:30 p.m. UTC | #1
On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> or USB hub, MAC passthrough address from BIOS should be
> checked if it had been used to avoid using on other dongles.
> 
> Currently builtin r8152 on Dock still can't be identified.
> First detected r8152 will use the MAC passthrough address.

I do have to wonder why you are doing this in the kernel, and not
using a udev rule? This seems to be policy, and policy does not belong
in the kernel.

   Andrew
Henning Schild Jan. 5, 2022, 5:40 p.m. UTC | #2
Am Wed, 5 Jan 2022 18:30:08 +0100
schrieb Andrew Lunn <andrew@lunn.ch>:

> On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> > When plugin multiple r8152 ethernet dongles to Lenovo Docks
> > or USB hub, MAC passthrough address from BIOS should be
> > checked if it had been used to avoid using on other dongles.
> > 
> > Currently builtin r8152 on Dock still can't be identified.
> > First detected r8152 will use the MAC passthrough address.  
> 
> I do have to wonder why you are doing this in the kernel, and not
> using a udev rule? This seems to be policy, and policy does not belong
> in the kernel.

Yes, the whole pass-thru story should not be a kernel feature in the
first place, i could not agree more, udev would be the way better place!
But it is part of the driver and Aaron did not introduce it but just
extend it.

Henning

>    Andrew
Andrew Lunn Jan. 5, 2022, 10:27 p.m. UTC | #3
On Wed, Jan 05, 2022 at 10:49:56PM +0100, Oliver Neukum wrote:
> 
> On 05.01.22 18:30, Andrew Lunn wrote:
> > On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> >> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> >> or USB hub, MAC passthrough address from BIOS should be
> >> checked if it had been used to avoid using on other dongles.
> >>
> >> Currently builtin r8152 on Dock still can't be identified.
> >> First detected r8152 will use the MAC passthrough address.
> > I do have to wonder why you are doing this in the kernel, and not
> > using a udev rule? This seems to be policy, and policy does not belong
> > in the kernel.
> Debatable. An ethernet NIC has to have a MAC. The kernel must
> provide one. That we should always take the one found in the
> device's ROM rather than the host's ROM is already a policy decision.

In general, it is a much longer list of places to find the MAC address
from. It could be in three different places in device tree, it could
be in ACPI in a similar number of places, it could be in NVMEM,
pointed to by device tree, the bootloader might of already programmed
the controller with its MAC address, etc, or if everything else fails,
it could be random.

So yes, the kernel will give it one. But if you want the interface to
have a specific MAC address, you probably should not be trusting the
kernel, given it has so many options.

	Andrew
Kai-Heng Feng Jan. 6, 2022, 2:10 a.m. UTC | #4
On Thu, Jan 6, 2022 at 6:27 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jan 05, 2022 at 10:49:56PM +0100, Oliver Neukum wrote:
> >
> > On 05.01.22 18:30, Andrew Lunn wrote:
> > > On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> > >> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> > >> or USB hub, MAC passthrough address from BIOS should be
> > >> checked if it had been used to avoid using on other dongles.
> > >>
> > >> Currently builtin r8152 on Dock still can't be identified.
> > >> First detected r8152 will use the MAC passthrough address.
> > > I do have to wonder why you are doing this in the kernel, and not
> > > using a udev rule? This seems to be policy, and policy does not belong
> > > in the kernel.
> > Debatable. An ethernet NIC has to have a MAC. The kernel must
> > provide one. That we should always take the one found in the
> > device's ROM rather than the host's ROM is already a policy decision.
>
> In general, it is a much longer list of places to find the MAC address
> from. It could be in three different places in device tree, it could
> be in ACPI in a similar number of places, it could be in NVMEM,
> pointed to by device tree, the bootloader might of already programmed
> the controller with its MAC address, etc, or if everything else fails,
> it could be random.
>
> So yes, the kernel will give it one. But if you want the interface to
> have a specific MAC address, you probably should not be trusting the
> kernel, given it has so many options.

Can udev in current form really handle the MAC race? Unless there's a
new uevent right before ndo_open() so udev can decide which MAC to
assign? Even with that, the race can still happen...

So what if we keep the existing behavior (i.e. copy MAC from ACPI),
and let userspace daemon like NetworkManager to give the second NIC
(r8152 in this case) a random MAC if the main NIC (I219 in this case)
is already in use? Considering the userspace daemon has the all the
information required and it's the policy maker here.

Kai-Heng

>
>         Andrew
>
>
Andrew Lunn Jan. 6, 2022, 1:27 p.m. UTC | #5
> Can udev in current form really handle the MAC race? Unless there's a
> new uevent right before ndo_open() so udev can decide which MAC to
> assign? Even with that, the race can still happen...

There will always be a race, since the kernel can start using the
interface before register_netdev() has even finished, before user
space is running. Take a look at how NFS root works.

But in general, you can change the MAC address at any time. Some MAC
drivers will return -EBUSY if the interface is up, but that is
generally artificial. After a change of MAC address ARP will time out
after a while and the link peers will get the new MAC address.

> 
> So what if we keep the existing behavior (i.e. copy MAC from ACPI),
> and let userspace daemon like NetworkManager to give the second NIC
> (r8152 in this case) a random MAC if the main NIC (I219 in this case)
> is already in use? Considering the userspace daemon has the all the
> information required and it's the policy maker here.

You should be thinking of this in more general terms. You want to
design a system that will work for any vendors laptop and dock.

You need to describe the two interfaces using some sort of bus
address, be it PCIe, USB, or a platform device address as used by
device tree etc.

Let the kernel do whatever it wants with MAC addresses for these two
interfaces. The only requirement you have is that the laptop internal
interface gets the vendor allocated MAC address, and that the dock get
some sort of MAC address, even if it is random.

On device creation, udev can check if it now has both interfaces? If
the internal interface is up, it is probably in use. Otherwise, take
its MAC address and assign it to the dock interface, and give the
internal interface a random MAC address, just in case.

You probably need to delay NetworkManager, systemd-networkkd,
/etc/network/interfaces etc, so that they don't do anything until
after udevd has settled, indicating all devices have probably been
found.

I suspect you will never get a 100% robust design, but you can
probably get it working enough for the cleaning staff and the CEO, who
have very simple setups. Power users are going to find all the corner
cases and will want to disable the udev rule.

     Andrew
Kai-Heng Feng Jan. 10, 2022, 3:39 a.m. UTC | #6
On Fri, Jan 7, 2022 at 9:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > You should be thinking of this in more general terms. You want to
> > > design a system that will work for any vendors laptop and dock.
> > >
> > > You need to describe the two interfaces using some sort of bus
> > > address, be it PCIe, USB, or a platform device address as used by
> > > device tree etc.
> > >
> > > Let the kernel do whatever it wants with MAC addresses for these two
> > > interfaces. The only requirement you have is that the laptop internal
> > > interface gets the vendor allocated MAC address, and that the dock get
> > > some sort of MAC address, even if it is random.
> >
> > Those laptops and docks are designed to have duplicated MACs. I don't
> > understand why but that's why Dell/HP/Lenovo did.
>
> But it also sounds like the design is broken. So the question is, is
> it possible to actually implement it correctly, without breaking
> networking for others with sane laptop/docks/USB dongles.

It's possible, just stick to whitelist and never over generalize the
device matching rule.

>
> > What if the kernel just abstract the hardware/firmware as intended, no
> > matter how stupid it is, and let userspace to make the right policy?
>
> Which is exactly what is being suggested here. The kernel gives the
> laptop internal interface its MAC address from ACPI or where ever, and
> the dock which has no MAC address gets a random MAC address. That is
> the normal kernel abstract. Userspace, in the form of udev, can then
> change the MAC addresses in whatever way it wants.

That's not what I mean. I mean the kernel should do what
firmware/hardware expects kernel should do - copy the MAC from ACPI to
the external NIC in the dock.
Then the userspace can assign a random MAC to external interface if
internal interface is already up.

>
> > But power users may also need to use corporate network to work as
> > Aaron mentioned.
> > Packets from unregistered MAC can be filtered under corporate network,
> > and that's why MAC pass-through is a useful feature that many business
> > laptops have.
>
> Depends on the cooperate network, but power users generally know more
> than the IT department, and will just make their machine work, copying
> the 802.3x certificate where ever it needs to go, us ebtables to
> mangle the MAC address, build their own little network with an RPi
> acting as a gateway doing NAT and MAC address translation, etc.

That's true, but as someone who work closely with other Distro folks,
we really should make this feature works for (hopefully) everyone.

Kai-Heng

>
>        Andrew
Oliver Neukum Jan. 10, 2022, 11:39 a.m. UTC | #7
On 05.01.22 16:14, Aaron Ma wrote:
> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> or USB hub, MAC passthrough address from BIOS should be
> checked if it had been used to avoid using on other dongles.
>
> Currently builtin r8152 on Dock still can't be identified.
> First detected r8152 will use the MAC passthrough address.
>
> v2:
> Skip builtin PCI MAC address which is share MAC address with
> passthrough MAC.
> Check thunderbolt based ethernet.
I am sorry and I may be dense, why are you comparing
MACs? The pass-through is done per machine. All you
nee is a global flag, or if you want to be even more
formal, a reference count.

    Regards
        Oliver
Mario Limonciello Jan. 11, 2022, 4:33 p.m. UTC | #8
On 1/11/2022 10:26, Jakub Kicinski wrote:
> On Tue, 11 Jan 2022 08:57:39 -0600 Limonciello, Mario wrote:
>> The important thing to remember is that many of these machines *don't*
>> have in-built network controller and rely upon a USB-c network adapter.
>>
>> I recall a few reasons.
>>
>> 1) Consistency with the UEFI network stack and dual booting Windows when
>> using the machine.  IOW 1 DHCP lease to one network controller, not one OS.
>>
>> 2) A (small) part of an onion that is network security.  It allows
>> administrators to allow-list or block-list controllers.
>>
>> The example I recall hearing is someone has their laptop stolen and
>> notifies I/T.  I/T removes the MAC address of the pass through address
>> from the allow-list and now that laptop can't use any hotel cubes for
>> accessing network resources.
>>
>> 3) Resource planning and management of hoteling resources.
>>
>> For example allow facilities to monitor whether users are reserving and
>> using the hoteling cubes they reserved.
> 
> Interesting, I haven't thought about use case (3).

These are just the cases I have from my memory when we kicked this off. 
  There may be others that are now used too.

> 
> Do you know how this is implemented on other platforms?


It's entirely OS independent - but presumes that there is a mapping of 
the pass through MAC address of the HW to a user account in the hoteling 
cube reservation software.

If you end up having only your pass through MAC used for Windows and 
UEFI your hoteling system might not work properly if your corporation 
also supports employees to use Linux and this feature was removed from 
the kernel.
Mario Limonciello Jan. 11, 2022, 4:54 p.m. UTC | #9
On 1/11/2022 10:43, Jakub Kicinski wrote:
> On Tue, 11 Jan 2022 10:33:39 -0600 Limonciello, Mario wrote:
>> If you end up having only your pass through MAC used for Windows and
>> UEFI your hoteling system might not work properly if your corporation
>> also supports employees to use Linux and this feature was removed from
>> the kernel.
> 
> Right, I think the utility of the feature is clear now. Let me clarify
> what I was after - 

Thanks, I was looped into this thread late so I didn't have any context.

You prompted me to look at this patch series on patchwork.

We talked about this when the initial patch series was developed and the 
intention was to align what Windows does.  That means that all dongles 
or docks with the appropriate effuse blown take the same pass through 
address.

Anything else and you lose the element of predictability and all those 
use cases I mentioned stop working.

> I was wondering which component is responsible for
> the address inheritance in Windows or UEFI. 

The Realtek driver for Windows and the Realtek DXE for UEFI.

> Is it also hardcoded into
> the realtek driver or is there a way to export the ACPI information to
> the network management component?

On Windows there is no indication outside of the driver that this 
feature has been used.  It's "invisible" to the user.

> 
> Also knowing how those OSes handle the new docks which don't have
> unique device IDs would obviously be great..

I'm sorry, can you give me some more context on this?  What unique 
device IDs?
Mario Limonciello Jan. 11, 2022, 5:10 p.m. UTC | #10
On 1/11/2022 11:06, Jakub Kicinski wrote:
> On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
>>> Also knowing how those OSes handle the new docks which don't have
>>> unique device IDs would obviously be great..
>>
>> I'm sorry, can you give me some more context on this?  What unique
>> device IDs?
> 
> We used to match the NICs based on their device ID. The USB NICs
> present in docks had lenovo as manufacturer and a unique device ID.
> Now reportedly the new docks are using generic realtek IDs so we have
> no way to differentiate "blessed" dock NICs from random USB dongles,
> and inheriting the address to all devices with the generic relatek IDs
> breaks setups with multiple dongles, e.g. Henning's setup. >
> If we know of a fuse that can be read on new docks that'd put us back
> in more comfortable position. If we need to execute random heuristics
> to find the "right NIC" we'd much rather have udev or NetworkManager
> or some other user space do that according to whatever policy it
> chooses.

I agree - this stuff in the kernel isn't supposed to be applying to 
anything other than the OEM dongles or docks.  If you can't identify 
them they shouldn't be in here.
Henning Schild Jan. 12, 2022, 7:21 p.m. UTC | #11
Am Tue, 11 Jan 2022 11:10:52 -0600
schrieb "Limonciello, Mario" <mario.limonciello@amd.com>:

> On 1/11/2022 11:06, Jakub Kicinski wrote:
> > On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:  
> >>> Also knowing how those OSes handle the new docks which don't have
> >>> unique device IDs would obviously be great..  
> >>
> >> I'm sorry, can you give me some more context on this?  What unique
> >> device IDs?  
> > 
> > We used to match the NICs based on their device ID. The USB NICs
> > present in docks had lenovo as manufacturer and a unique device ID.
> > Now reportedly the new docks are using generic realtek IDs so we
> > have no way to differentiate "blessed" dock NICs from random USB
> > dongles, and inheriting the address to all devices with the generic
> > relatek IDs breaks setups with multiple dongles, e.g. Henning's
> > setup. > If we know of a fuse that can be read on new docks that'd
> > put us back in more comfortable position. If we need to execute
> > random heuristics to find the "right NIC" we'd much rather have
> > udev or NetworkManager or some other user space do that according
> > to whatever policy it chooses.  
> 
> I agree - this stuff in the kernel isn't supposed to be applying to 
> anything other than the OEM dongles or docks.  If you can't identify 
> them they shouldn't be in here.

Not sure we can really get to a proper solution here. The one revert
for Lenovo will solve my very issue. And on top i was lucky enough to
being able to disable pass-thru in the BIOS.

From what the Lenovo vendor docs seem to suggest it is about PXE ...
meaning the BIOS will do the spoofing, how it does that is unclear. Now
an OS can try to keep it up but probably should not try to do anything
on its own ... or do the additional bits in user-space and not the
kernel.

Thinking about PXE we do not just have the kernel, but also multiple
potential bootloaders. All would need to inherit the spoofed MAC from a
potential predecessor having applied spoofing, and support USB and
network ... that is not realistic!

Going back to PXE ... says nothing about OS operation really. Say a
BIOS decides to spoof when booting ... that say nothing on how to deal
with hot-plugged devices which die BIOS did not even see. But we have
code for such hot-plug spoofing in the kernel .. where vendors like
Lenovo talk about PXE (only?)

The whole story seems too complicated and not really explained or
throught through. If the vendors can explain stuff the kernel can
probably cater ... but user-land would still be the better place.

I will not push for more reverts. But more patches in the direction
should be questioned really hard! And even if we get "proper device
matching" we will only cater for "vendor lock-in". It is not clear why
that strange feature will only apply if the dock if from the same
vendor as the laptop. Applying it on all USB NICs is clearly going too
far, that will only work with IT department highlander policies like
"there must only be one NIC".

So from my point it is solved with the one Lenovo-related revert. Any
future pass-thru patches get a NACK and any reverts targeting other
vendors get an ACK. But feel free to Cc me when such things happen in
the future.

regards,
Henning
Mario Limonciello Jan. 12, 2022, 7:27 p.m. UTC | #12
[Public]

> -----Original Message-----
> From: Henning Schild <henning.schild@siemens.com>
> Sent: Wednesday, January 12, 2022 13:21
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; Kai-Heng Feng
> <kai.heng.feng@canonical.com>; Andrew Lunn <andrew@lunn.ch>; Oliver
> Neukum <oneukum@suse.com>; Aaron Ma <aaron.ma@canonical.com>; linux-
> usb@vger.kernel.org; netdev@vger.kernel.org; davem@davemloft.net;
> hayeswang@realtek.com; tiwai@suse.de
> Subject: Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough
> address
> 
> Am Tue, 11 Jan 2022 11:10:52 -0600
> schrieb "Limonciello, Mario" <mario.limonciello@amd.com>:
> 
> > On 1/11/2022 11:06, Jakub Kicinski wrote:
> > > On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
> > >>> Also knowing how those OSes handle the new docks which don't have
> > >>> unique device IDs would obviously be great..
> > >>
> > >> I'm sorry, can you give me some more context on this?  What unique
> > >> device IDs?
> > >
> > > We used to match the NICs based on their device ID. The USB NICs
> > > present in docks had lenovo as manufacturer and a unique device ID.
> > > Now reportedly the new docks are using generic realtek IDs so we
> > > have no way to differentiate "blessed" dock NICs from random USB
> > > dongles, and inheriting the address to all devices with the generic
> > > relatek IDs breaks setups with multiple dongles, e.g. Henning's
> > > setup. > If we know of a fuse that can be read on new docks that'd
> > > put us back in more comfortable position. If we need to execute
> > > random heuristics to find the "right NIC" we'd much rather have
> > > udev or NetworkManager or some other user space do that according
> > > to whatever policy it chooses.
> >
> > I agree - this stuff in the kernel isn't supposed to be applying to
> > anything other than the OEM dongles or docks.  If you can't identify
> > them they shouldn't be in here.
> 
> Not sure we can really get to a proper solution here. The one revert
> for Lenovo will solve my very issue. And on top i was lucky enough to
> being able to disable pass-thru in the BIOS.
> 
> From what the Lenovo vendor docs seem to suggest it is about PXE ...
> meaning the BIOS will do the spoofing, how it does that is unclear. Now
> an OS can try to keep it up but probably should not try to do anything
> on its own ... or do the additional bits in user-space and not the
> kernel.
> 
> Thinking about PXE we do not just have the kernel, but also multiple
> potential bootloaders. All would need to inherit the spoofed MAC from a
> potential predecessor having applied spoofing, and support USB and
> network ... that is not realistic!
> 
> Going back to PXE ... says nothing about OS operation really. Say a
> BIOS decides to spoof when booting ... that say nothing on how to deal
> with hot-plugged devices which die BIOS did not even see. But we have
> code for such hot-plug spoofing in the kernel .. where vendors like
> Lenovo talk about PXE (only?)

Something that would probably be interesting to check is whether the
BIOS uses pass through MAC on anything as well or it has some criteria
that decides to apply it that the kernel doesn't know about.

> 
> The whole story seems too complicated and not really explained or
> throught through. If the vendors can explain stuff the kernel can
> probably cater ... but user-land would still be the better place.
> 
> I will not push for more reverts. But more patches in the direction
> should be questioned really hard! And even if we get "proper device
> matching" we will only cater for "vendor lock-in". It is not clear why
> that strange feature will only apply if the dock if from the same
> vendor as the laptop. Applying it on all USB NICs is clearly going too
> far, that will only work with IT department highlander policies like
> "there must only be one NIC".
> 
> So from my point it is solved with the one Lenovo-related revert. Any
> future pass-thru patches get a NACK and any reverts targeting other
> vendors get an ACK. But feel free to Cc me when such things happen in
> the future.
> 

KH & Aaron - can you please talk to Lenovo about making sure that there
is a way to distinguish between devices that should get pass through or
shouldn't and to document that?  

I think that a policy that it should be a NACK for anything else general
makes sense.

> regards,
> Henning
Aaron Ma Jan. 13, 2022, 3:23 a.m. UTC | #13
On 1/13/22 03:27, Limonciello, Mario wrote:
> [Public]
>
>> -----Original Message-----
>> From: Henning Schild <henning.schild@siemens.com>
>> Sent: Wednesday, January 12, 2022 13:21
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>; Kai-Heng Feng
>> <kai.heng.feng@canonical.com>; Andrew Lunn <andrew@lunn.ch>; Oliver
>> Neukum <oneukum@suse.com>; Aaron Ma <aaron.ma@canonical.com>; linux-
>> usb@vger.kernel.org; netdev@vger.kernel.org; davem@davemloft.net;
>> hayeswang@realtek.com; tiwai@suse.de
>> Subject: Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough
>> address
>>
>> Am Tue, 11 Jan 2022 11:10:52 -0600
>> schrieb "Limonciello, Mario" <mario.limonciello@amd.com>:
>>
>>> On 1/11/2022 11:06, Jakub Kicinski wrote:
>>>> On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
>>>>>> Also knowing how those OSes handle the new docks which don't have
>>>>>> unique device IDs would obviously be great..
>>>>> I'm sorry, can you give me some more context on this?  What unique
>>>>> device IDs?
>>>> We used to match the NICs based on their device ID. The USB NICs
>>>> present in docks had lenovo as manufacturer and a unique device ID.
>>>> Now reportedly the new docks are using generic realtek IDs so we
>>>> have no way to differentiate "blessed" dock NICs from random USB
>>>> dongles, and inheriting the address to all devices with the generic
>>>> relatek IDs breaks setups with multiple dongles, e.g. Henning's
>>>> setup. > If we know of a fuse that can be read on new docks that'd
>>>> put us back in more comfortable position. If we need to execute
>>>> random heuristics to find the "right NIC" we'd much rather have
>>>> udev or NetworkManager or some other user space do that according
>>>> to whatever policy it chooses.
>>> I agree - this stuff in the kernel isn't supposed to be applying to
>>> anything other than the OEM dongles or docks.  If you can't identify
>>> them they shouldn't be in here.
>> Not sure we can really get to a proper solution here. The one revert
>> for Lenovo will solve my very issue. And on top i was lucky enough to
>> being able to disable pass-thru in the BIOS.
>>
>>  From what the Lenovo vendor docs seem to suggest it is about PXE ...
>> meaning the BIOS will do the spoofing, how it does that is unclear. Now
>> an OS can try to keep it up but probably should not try to do anything
>> on its own ... or do the additional bits in user-space and not the
>> kernel.
>>
>> Thinking about PXE we do not just have the kernel, but also multiple
>> potential bootloaders. All would need to inherit the spoofed MAC from a
>> potential predecessor having applied spoofing, and support USB and
>> network ... that is not realistic!
>>
>> Going back to PXE ... says nothing about OS operation really. Say a
>> BIOS decides to spoof when booting ... that say nothing on how to deal
>> with hot-plugged devices which die BIOS did not even see. But we have
>> code for such hot-plug spoofing in the kernel .. where vendors like
>> Lenovo talk about PXE (only?)
> Something that would probably be interesting to check is whether the
> BIOS uses pass through MAC on anything as well or it has some criteria
> that decides to apply it that the kernel doesn't know about.
>
>> The whole story seems too complicated and not really explained or
>> throught through. If the vendors can explain stuff the kernel can
>> probably cater ... but user-land would still be the better place.
>>
>> I will not push for more reverts. But more patches in the direction
>> should be questioned really hard! And even if we get "proper device
>> matching" we will only cater for "vendor lock-in". It is not clear why
>> that strange feature will only apply if the dock if from the same
>> vendor as the laptop. Applying it on all USB NICs is clearly going too
>> far, that will only work with IT department highlander policies like
>> "there must only be one NIC".
>>
>> So from my point it is solved with the one Lenovo-related revert. Any
>> future pass-thru patches get a NACK and any reverts targeting other
>> vendors get an ACK. But feel free to Cc me when such things happen in
>> the future.
>>
> KH & Aaron - can you please talk to Lenovo about making sure that there
> is a way to distinguish between devices that should get pass through or
> shouldn't and to document that?
>
> I think that a policy that it should be a NACK for anything else general
> makes sense.

Sorry for my previous patch.
Before made that patch I already discussed with Lenovo.
And didn't get any other opinion. The solution is from a discussion with them.

This info had been forward to them too.

Aaron

>
>> regards,
>> Henning
Aaron Ma Jan. 27, 2022, 2:51 a.m. UTC | #14
Hi all,

Realtek 8153BL can be identified by the following code from Realtek Outbox driver:
} else if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {

I will suggest Realtek to send out this change for review.

Thanks,
Aaron

On 1/13/22 11:23, Aaron Ma wrote:
> Before made that patch I already discussed with Lenovo.
> And didn't get any other opinion. The solution is from a discussion with them.
> 
> This info had been forward to them too.
Hayes Wang Jan. 27, 2022, 8:06 a.m. UTC | #15
Aaron Ma <aaron.ma@canonical.com>
> Sent: Thursday, January 27, 2022 10:52 AM
[...]
> Hi all,
> 
> Realtek 8153BL can be identified by the following code from Realtek Outbox
> driver:
> } else if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {
> 
> I will suggest Realtek to send out this change for review.

I don't think the feature of MAC passthrough address is maintained
by Realtek. Especially, there is no uniform way about it. The
different companies have to maintain their own ways by themselves.

Realtek could provide the method of finding out the specific device
for Lenovo. You could check USB OCP 0xD81F bit 3. For example,

	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
		/* This is the RTL8153B for Lenovo. */
	}

Best Regards,
Hayes
Aaron Ma Jan. 27, 2022, 8:13 a.m. UTC | #16
On 1/27/22 16:06, Hayes Wang wrote:
> I don't think the feature of MAC passthrough address is maintained
> by Realtek. Especially, there is no uniform way about it. The
> different companies have to maintain their own ways by themselves.
> 
> Realtek could provide the method of finding out the specific device
> for Lenovo. You could check USB OCP 0xD81F bit 3. For example,
> 
> 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> 	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
> 		/* This is the RTL8153B for Lenovo. */
> 	}
> 

May I use the code from Realtek Outbox driver to implement the MAPT?

If so, allow me to write a patch and send here to review.

Thanks,
Aaron


> Best Regards,
> Hayes
Hayes Wang Jan. 27, 2022, 8:42 a.m. UTC | #17
Aaron Ma <aaron.ma@canonical.com>
> Sent: Thursday, January 27, 2022 4:13 PM
[...]
> > I don't think the feature of MAC passthrough address is maintained
> > by Realtek. Especially, there is no uniform way about it. The
> > different companies have to maintain their own ways by themselves.
> >
> > Realtek could provide the method of finding out the specific device
> > for Lenovo. You could check USB OCP 0xD81F bit 3. For example,
> >
> > 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> > 	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
> > 		/* This is the RTL8153B for Lenovo. */
> > 	}
> >
> 
> May I use the code from Realtek Outbox driver to implement the MAPT?
> 
> If so, allow me to write a patch and send here to review.

Sure.

However, the outbox driver has a mistake.
The mac_obj_name with "\\_SB.AMAC" is used by Dell.
I think the device of Lenovo should use "\\MACA" only. Right?

The easiest way is to set tp->lenovo_macpassthru for RTL8153BL.
For example,

	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3)))
		tp->lenovo_macpassthru = 1;

Best Regards,
Hayes
Andrew Lunn Jan. 27, 2022, 12:53 p.m. UTC | #18
On Thu, Jan 27, 2022 at 08:06:20AM +0000, Hayes Wang wrote:
> Aaron Ma <aaron.ma@canonical.com>
> > Sent: Thursday, January 27, 2022 10:52 AM
> [...]
> > Hi all,
> > 
> > Realtek 8153BL can be identified by the following code from Realtek Outbox
> > driver:
> > } else if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {
> > 
> > I will suggest Realtek to send out this change for review.
> 
> I don't think the feature of MAC passthrough address is maintained
> by Realtek. Especially, there is no uniform way about it. The
> different companies have to maintain their own ways by themselves.
> 
> Realtek could provide the method of finding out the specific device
> for Lenovo. You could check USB OCP 0xD81F bit 3. For example,
> 
> 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> 	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
> 		/* This is the RTL8153B for Lenovo. */
> 	}

Is there a documented meaning for this bit? Realtek have allocated
this bit to Lenovo?

Why is it guaranteed that no other devices manufactured by anybody
else will not have this bit set? That is the real question here. Does
this give false positive where you break the networking for some
random USB dongle.

     Andrew
diff mbox series

Patch

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f9877a3e83ac..2483dc421dff 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -25,6 +25,7 @@ 
 #include <linux/atomic.h>
 #include <linux/acpi.h>
 #include <linux/firmware.h>
+#include <linux/pci.h>
 #include <crypto/hash.h>
 #include <linux/usb/r8152.h>
 
@@ -1605,6 +1606,7 @@  static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	char *mac_obj_name;
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
+	struct net_device *ndev;
 
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
@@ -1662,6 +1664,19 @@  static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 		ret = -EINVAL;
 		goto amacout;
 	}
+	rcu_read_lock();
+	for_each_netdev_rcu(&init_net, ndev) {
+		if (ndev->dev.parent && dev_is_pci(ndev->dev.parent) &&
+				!pci_is_thunderbolt_attached(to_pci_dev(ndev->dev.parent)))
+			continue;
+		if (strncmp(buf, ndev->dev_addr, 6) == 0) {
+			ret = -EINVAL;
+			rcu_read_unlock();
+			goto amacout;
+		}
+	}
+	rcu_read_unlock();
+
 	memcpy(sa->sa_data, buf, 6);
 	netif_info(tp, probe, tp->netdev,
 		   "Using pass-thru MAC addr %pM\n", sa->sa_data);