netdev - 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 14:02:12 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukasz 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

> >> +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.
> 
> Good point. What is your recommendation for the userland facing
> interface, so that future changes will be least disruptive?

I would KISS and just validate data is 0 or 1, and leave it at that.
That then later gives you the option to have other values, if that is
ever interesting.

     Andrew