linux-kernel - Re: [PATCH v5 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Nov 2020 03:42:11 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Łukasz Stelmach <l.stelmach@...sung.com>
Cc:     jim.cromie@...il.com, Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Bartłomiej Żolnierkiewicz 
        <b.zolnierkie@...sung.com>
Subject: Re: [PATCH v5 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter
 Driver

> +config SPI_AX88796C_COMPRESSION
> +	bool "SPI transfer compression"
> +	default n
> +	depends on SPI_AX88796C
> +	help
> +	  Say Y here to enable SPI transfer compression. It saves up
> +	  to 24 dummy cycles during each transfer which may noticably
> +	  speed up short transfers. This sets the default value that is
> +	  inherited by network interfecase during probe. It can be

interface


> +	  changed in run time via spi-compression ethtool tunable.

changed _at_ run time...

> +static int
> +ax88796c_set_tunable(struct net_device *ndev, const struct ethtool_tunable *tuna,
> +		     const void *data)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +
> +	switch (tuna->id) {
> +	case ETHTOOL_SPI_COMPRESSION:
> +		if (netif_running(ndev))
> +			return -EBUSY;
> +		ax_local->capabilities &= ~AX_CAP_COMP;
> +		ax_local->capabilities |= *(u32 *)data ? AX_CAP_COMP : 0;

You should probably validate here that data is 0 or 1. That is what
ax88796c_get_tunable() will return.

It seems like this controls two hardware bits:

SPICR_RCEN | SPICR_QCEN

Maybe at some point it would make sense to allow these bits to be set
individually? If you never validate the tunable, you cannot make use
of other values to control the bits individually.

   Andrew