netdev - Re: [PATCH ethtool v2] 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: <20220821232811.a2dlidrm5i62n6yu@lion.mk-sys.cz>
Date:   Mon, 22 Aug 2022 01:28:11 +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 v2] ethtool: fix EEPROM byte write

On Fri, Aug 19, 2022 at 12:10:49PM +0200, Tomasz Moń wrote:
> ethtool since version 1.8 supports EEPROM byte write:
>   # ethtool -E DEVNAME [ magic N ] [ offset N ] [ value N ]
> 
> ethtool 2.6.33 added EEPROM block write:
>   # ethtool -E ethX [ magic N ] [ offset N ] [ length N ] [ value N ]
> 
> EEPROM block write introduced in 2.6.33 is backwards compatible, i.e.
> when value is specified the length is forced to 1 (commandline length
> value is ignored).
> 
> The byte write behaviour changed in ethtool 5.9 where the value write
> only works when value parameter is specified together with length 1.
> While byte writes to any offset other than 0, without length 1, simply
> fail with "offset & length out of bounds" error message, writing value
> to offset 0 basically erased whole EEPROM. That is, the provided byte
> value was written at offset 0, but the rest of the EEPROM was set to 0.
> 
> Fix the issue by setting length to 1 when value is specified and length
> is omitted. Exit with error if length is specified to value other than 1
> and value is specified.
> 
> Fixes: 923c3f51c444 ("ioctl: check presence of eeprom length argument properly")
> Signed-off-by: Tomasz Moń <tomasz.mon@...lingroup.com>
> ---
> changes in v2:
>   - set the length to 1 only if not specified by user
>   - exit with error if length is not 1
> 
> v1: https://lore.kernel.org/netdev/20220819062933.1155112-1-tomasz.mon@camlingroup.com/
> ---
>  ethtool.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 89613ca..7b400da 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3529,12 +3529,16 @@ static int do_seeprom(struct cmd_context *ctx)
>  		return 74;
>  	}
>  
> -	if (seeprom_value_seen)
> +	if (seeprom_value_seen && !seeprom_length_seen)
>  		seeprom_length = 1;
> -
> -	if (!seeprom_length_seen)
> +	else if (!seeprom_length_seen)
>  		seeprom_length = drvinfo.eedump_len;

It would probably look a bit nicer like this:

	if (!seeprom_length_seen)
		seeprom_length = seeprom_value_seen ? 1 : drvinfo.eedump_len;

but that's just matter of taste so let's take it as it is.

Michal

>  
> +	if (seeprom_value_seen && (seeprom_length != 1)) {
> +		fprintf(stderr, "value requires length 1\n");
> +		return 1;
> +	}
> +
>  	if (drvinfo.eedump_len < seeprom_offset + seeprom_length) {
>  		fprintf(stderr, "offset & length out of bounds\n");
>  		return 1;
> -- 
> 2.25.1
> 

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