netdev - Re: [PATCH ethtool] ethtool: fix EEPROM byte write

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: <20220819090637.3u7o3xmuafftbzlf@lion.mk-sys.cz>
Date:   Fri, 19 Aug 2022 11:06:37 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Tomasz Moń <tomasz.mon@...lingroup.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Krzysztof Drobiński 
        <k.drobinski@...lintechnologies.com>
Subject: Re: [PATCH ethtool] ethtool: fix EEPROM byte write

On Fri, Aug 19, 2022 at 10:58:19AM +0200, Tomasz Moń wrote:
> On Fri, 2022-08-19 at 10:27 +0200, Michal Kubecek wrote:
> > On Fri, Aug 19, 2022 at 08:29:33AM +0200, Tomasz Moń wrote:
> > > @@ -3531,8 +3531,7 @@ static int do_seeprom(struct cmd_context *ctx)
> > >  
> > >  	if (seeprom_value_seen)
> > >  		seeprom_length = 1;
> > > -
> > > -	if (!seeprom_length_seen)
> > > +	else if (!seeprom_length_seen)
> > >  		seeprom_length = drvinfo.eedump_len;
> > >  
> > >  	if (drvinfo.eedump_len < seeprom_offset + seeprom_length) {
> > 
> > I don't like the idea of silently ignoring the length parameter if value
> > is used. We should rather issue an error for invalid combination of
> > parameters, i.e. value present and length different from 1.
> 
> This patch simply restores the way ethtool 2.6.33 - 5.8 worked.

Right but when have to touch the code anyway, let's improve the
behaviour.

> Setting length to 1 matches ethtool man page description:
> > If value is specified, changes EEPROM byte for the specified network device.
> > offset and value specify which byte and it's new value.
> 
> What about changing the code to default to length 1 if value is
> provided without length (so scripts relying on the way ethtool 1.8 up
> to 5.8 works without any change), but report error if user provides
> both value and length other than 1?

Yes, that's exactly what I meant. Using length 1 as default if length is
omitted and value present is perfectly fine, what I did not like was the
idea of silently adjusting invalid combinations of parameters (value
present and length different from 1).

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)