RE: [Intel-wired-lan] [PATCH] igb: Add MII write support

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On
> Behalf Of Jacob Keller
> Sent: Wednesday, June 5, 2024 11:17 PM
> To: Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>; Jackie Jone
> <Jackie.Jone@xxxxxxxxxxxxxxxxxxx>; davem@xxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Nguyen,
> Anthony L <anthony.l.nguyen@xxxxxxxxx>; intel-wired-
> lan@xxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx
> Subject: Re: [Intel-wired-lan] [PATCH] igb: Add MII write support
> 
> 
> 
> On 6/5/2024 2:10 PM, Chris Packham wrote:
> >
> > On 6/06/24 08:51, Jacob Keller wrote:
> >>
> >> On 6/3/2024 8:10 PM, jackie.jone@xxxxxxxxxxxxxxxxxxx wrote:
> >>> From: Jackie Jone <jackie.jone@xxxxxxxxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>>   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?
> >>
> >> 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?
> >
> > CAP_NET_ADMIN is enforced via dev_ioctl() so it should already be
> > restricted to users with that capability.
> 
> Ok good. That at least limits this so that random users can't cause
> any side effects.
> 
I'm pretty sure from experience that even root applications will start cause nvmupdate issues.

> I'm not super familiar with what can be affected by writing the MII
> registers. I'm also not sure what the community thinks of exposing
> such access directly.
> 
> From the description this is intended to use for debugging and
> testing purposes?