netdev - RE: [PATCH net-next 1/1] net: stmmac: add check for advertising linkmode request for set-eee

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DM6PR11MB330605F0FDCE81F8379ACFEBB9A7A@DM6PR11MB3306.namprd11.prod.outlook.com>
Date: Wed, 1 Nov 2023 05:41:48 +0000
From: "Gan, Yi Fang" <yi.fang.gan@...el.com>
To: Russell King <linux@...linux.org.uk>
CC: Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu
	<joabreu@...opsys.com>, "David S . Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-stm32@...md-mailman.stormreply.com"
	<linux-stm32@...md-mailman.stormreply.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Looi, Hong Aun" <hong.aun.looi@...el.com>,
	"Voon, Weifeng" <weifeng.voon@...el.com>, "Song, Yoong Siang"
	<yoong.siang.song@...el.com>, "Ahmad Tarmizi, Noor Azura"
	<noor.azura.ahmad.tarmizi@...el.com>
Subject: RE: [PATCH net-next 1/1] net: stmmac: add check for advertising
 linkmode request for set-eee



> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: Tuesday, October 31, 2023 5:09 PM
> To: Gan, Yi Fang <yi.fang.gan@...el.com>
> Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>; Jose Abreu
> <joabreu@...opsys.com>; David S . Miller <davem@...emloft.net>; Eric
> Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo
> Abeni <pabeni@...hat.com>; Maxime Coquelin
> <mcoquelin.stm32@...il.com>; netdev@...r.kernel.org; linux-stm32@...md-
> mailman.stormreply.com; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; Looi, Hong Aun <hong.aun.looi@...el.com>; Voon,
> Weifeng <weifeng.voon@...el.com>; Song, Yoong Siang
> <yoong.siang.song@...el.com>; Ahmad Tarmizi, Noor Azura
> <noor.azura.ahmad.tarmizi@...el.com>
> Subject: Re: [PATCH net-next 1/1] net: stmmac: add check for advertising
> linkmode request for set-eee
> 
> On Tue, Oct 31, 2023 at 08:44:23AM +0000, Gan, Yi Fang wrote:
> > Hi Russell King,
> >
> > > Why should this functionality be specific to stmmac?
> > This functionality is not specific to stmmac but other drivers can
> > have their  own implementation.
> > (e.g.
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ql
> > ogic/qede/qede_ethtool.c#L1855)
> 
> This is probably wrong (see below.)
> 
> >
> > > Why do we need this?
> > Current implementation will not take any effect if user enters
> > unsupported value but user might not aware. With this, an error will be
> prompted if unsupported value is given.
> 
> Why can't the user read back what settings were actually set like the other
> ethtool APIs? This is how ETHTOOL_GLINKSETTINGS works.
> 
For current implementation, the behaviour of "fail to set" and
"set successfully" are the same from user's perspective. 
But yes, user have responsibility to read back and check the setting.

> > > What is wrong with the checking and masking that phylib is doing?
> > Nothing wrong with the phylib but there is no error return back to
> > ethtool commands if unsupported value is given.
> 
> Maybe because that is the correct implementation?
> 
Yes, I agree with this.

> > > Why should we trust the value in edata->supported provided by the user?
> > The edata->supported is getting from the current setting and the value is set
> upon bootup.
> > Users are not allowed to change it.
> 
> "not allowed" but there is nothing that prevents it. So an easy way to bypass
> your check is:
> 
> 	struct ethtool_eee eeecmd;
> 
> 	eeecmd.cmd = ETHTOOL_GEEE;
> 	send_ioctl(..., &eeecmd);
> 
> 	eeecmd.cmd = ETHTOOL_SEEE;
> 	eeecmd.supported = ~0;
> 	eeecmd.advertised = ~0;
> 	error = send_ioctl(..., &eeecmd);
> 
> and that won't return any error. So your check is weak at best, and relies upon
> the user doing the right thing.
> 
Thank you for the example.

> > > Sorry, but no. I see no reason that this should be done, especially not in the
> stmmac driver.
> > I understand your reasoning. From your point of view, is this kind of
> > error message/ error handling not needed?
> 
> It is not - ethtool APIs don't return errors if the advertise mask is larger than the
> supported mask - they merely limit to what is supported and set that. When
> subsequently querying the settings, they return what is actually set (so the
> advertise mask will always be a subset of the supported mask at that point.)
> 
> So, if in userspace you really want to know if some modes were dropped, then
> you have to do a set-get-check sequence.
>
Thank you for all the feedbacks and explanations. It was a great discussion.
I understand your concerns and reasonings. Let's close the review for this patch. 

Thanks.
Best Regards,
Fang