Re: [PATCH] igb: Add MII write support

[Jacob Keller <jacob.e.keller@intel.com>]

On 6/6/2024 8:41 AM, Andrew Lunn wrote:
> On Wed, Jun 05, 2024 at 01:51:24PM -0700, Jacob Keller wrote:
>>
>>
>> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote:
>>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>>>
>>> To facilitate running PHY parametric tests, add support for the SIOCSMIIREG
>>> ioctl. This allows a userspace application to write to the PHY registers
>>> to enable the test modes.
>>>
>>> Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>>> ---
>>>  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>> index 03a4da6a1447..7fbfcf01fbf9 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>>>  			return -EIO;
>>>  		break;
>>>  	case SIOCSMIIREG:
>>> +		if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F,
>>> +				     data->val_in))
>>> +			return -EIO;
>>> +		break;
>>
>> A handful of drivers seem to expose this. What are the consequences of
>> exposing this ioctl? What can user space do with it?
> 
> User space can break the PHY configuration, cause the link to fail,
> all behind the MAC drivers back. The generic version of this call
> tries to see what registers are being written, and update state:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy.c#L325
> 
> But you can still break it.
> 

Yea, its extremely easy to break things if you don't know what you're
doing here. So its more a question of "are we ok exposing yet another
way root can brick things?"

>> It looks like a few drivers also check something like CAP_NET_ADMIN to
>> avoid allowing write access to all users. Is that enforced somewhere else?
> 
> Only root is allowed to use it. So it is a classic 'You have the
> option to shoot yourself in the foot'.
> 

I don't have an objection to enabling this myself, but I do want to be
cognizant of the way it is viewed in the wider community.

> For the use case being talked about here, there has been a few emails
> one the list about implementing the IEEE 802.3 test modes. But nobody
> has actually got around to doing it. Not that it would help in this
> case, Intel don't use the Linux PHY drivers, which is where i would
> expect to see such code implemented first. Maybe if the "Great Intel
> Ethernet driver refactoring" makes progress, it could swap to using
> the Linux PHY drivers.
> 
>       Andrew

I remember this coming up several times in the past. I've always tried
to push for it, but so far unsuccessfully.