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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 20 Sep 2022 01:49:24 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add
 functionality to get RMON

On Mon, Sep 19, 2022 at 01:08:45PM +0200, Mattias Forsblad wrote:
> @@ -255,6 +299,15 @@ int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>  	chip->rmu.smi_ops = chip->smi_ops;
>  	chip->rmu.ds_ops = chip->ds->ops;
>  
> +	/* Change rmu ops with our own pointer */
> +	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
> +	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;

The patch splitting is still so poor, that I can't even complain about
this bug properly, since no one uses this pointer for now (and when it
will be used, it will not be obvious how it's assigned).

In short, it's called like this:

static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
					uint64_t *data)
{
	struct mv88e6xxx_chip *chip = ds->priv;

	chip->smi_ops->get_rmon(chip, port, data);
}

When the switch doesn't support RMU operations, or when the RMU setup
simply failed, "ethtool -S" will dereference a very NULL pointer during
that indirect call, because mv88e6xxx_rmu_setup() is unconditionally
called for every switch during setup. Probably not a wise choice to
start off with the RMU ops for ethtool -S.

Also, not clear, if RMU fails, why we don't make an effort to fall back
to MDIO for that user space request.

> +
> +	/* Also change get stats strings for our own */

Who is "our own", and implicitly, who are they? You'd expect that there
aren't tribalist factions within the same driver.

> +	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
> +	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
> +	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;
> +

Those who cast a const to a non-const pointer and proceed to modify the
read-only structure behind it should go to patchwork arrest for one week.

>  	/* Start disabled, we'll enable RMU in master_state_change */
>  	if (chip->info->ops->rmu_disable)
>  		return chip->info->ops->rmu_disable(chip);
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists