netdev - Re: [PATCH v2 net-next 2/2] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

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]

Date:   Thu, 20 May 2021 23:35:43 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
        Guenter Roeck <linux@...ck-us.net>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v2 net-next 2/2] net: dsa: sja1105: adapt to a SPI
 controller with a limited max transfer size

On Thu, May 20, 2021 at 11:02:23PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> The static config of the sja1105 switch is a long stream of bytes which
> is programmed to the hardware in chunks (portions with the chip select
> continuously asserted) of max 256 bytes each.
> 
> Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> bridge, cannot keep the chip select asserted for that long.
> The spi_max_transfer_size() and spi_max_message_size() functions are how
> the controller can impose its hardware limitations upon the SPI
> peripheral driver.
> 
> The sja1105 sends its static config to the SPI master in chunks, and
> each chunk is a spi_message composed of 2 spi_transfers: the buffer with
> the data and a preceding buffer with the SPI access header. Both buffers
> must be smaller than the transfer limit, and their sum must be smaller
> than the message limit.
> 
> Regression-tested on a switch connected to a controller with no
> limitations (spi-fsl-dspi) as well as with one with caps for both
> max_transfer_size and max_message_size (spi-sc18is602).
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/net/dsa/sja1105/sja1105_spi.c | 30 ++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
> index 8746e3f158a0..7bcf2e419037 100644
> --- a/drivers/net/dsa/sja1105/sja1105_spi.c
> +++ b/drivers/net/dsa/sja1105/sja1105_spi.c
> @@ -40,19 +40,35 @@ static int sja1105_xfer(const struct sja1105_private *priv,
>  			size_t len, struct ptp_system_timestamp *ptp_sts)
>  {
>  	u8 hdr_buf[SJA1105_SIZE_SPI_MSG_HEADER] = {0};
> -	struct sja1105_chunk chunk = {
> -		.len = min_t(size_t, len, SJA1105_SIZE_SPI_MSG_MAXLEN),
> -		.reg_addr = reg_addr,
> -		.buf = buf,
> -	};
>  	struct spi_device *spi = priv->spidev;
>  	struct spi_transfer xfers[2] = {0};
>  	struct spi_transfer *chunk_xfer;
>  	struct spi_transfer *hdr_xfer;
> +	struct sja1105_chunk chunk;
> +	ssize_t xfer_len;
>  	int num_chunks;
>  	int rc, i = 0;
>  
> -	num_chunks = DIV_ROUND_UP(len, SJA1105_SIZE_SPI_MSG_MAXLEN);
> +	/* One spi_message is composed of two spi_transfers: a small one for
> +	 * the message header and another one for the current chunk of the
> +	 * packed buffer.
> +	 * Check that the restrictions imposed by the SPI controller are
> +	 * respected: the chunk buffer is smaller than the max transfer size,
> +	 * and the total length of the chunk plus its message header is smaller
> +	 * than the max message size.
> +	 */
> +	xfer_len = min_t(ssize_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
> +			 spi_max_transfer_size(spi));
> +	xfer_len = min_t(ssize_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
> +			 spi_max_message_size(spi) - SJA1105_SIZE_SPI_MSG_HEADER);
> +	if (xfer_len < 0)
> +		return -ERANGE;

I've introduced a bug here when spi_max_message_size returns SIZE_MAX
which is of the unsigned size_t type. Converted to ssize_t it's negative,
so it triggers the negative check...

Please wait until I send a v3 with this fixed. Thanks.

> +
> +	num_chunks = DIV_ROUND_UP(len, xfer_len);
> +
> +	chunk.reg_addr = reg_addr;
> +	chunk.buf = buf;
> +	chunk.len = min_t(size_t, len, xfer_len);
>  
>  	hdr_xfer = &xfers[0];
>  	chunk_xfer = &xfers[1];
> @@ -104,7 +120,7 @@ static int sja1105_xfer(const struct sja1105_private *priv,
>  		chunk.buf += chunk.len;
>  		chunk.reg_addr += chunk.len / 4;
>  		chunk.len = min_t(size_t, (ptrdiff_t)(buf + len - chunk.buf),
> -				  SJA1105_SIZE_SPI_MSG_MAXLEN);
> +				  xfer_len);
>  
>  		rc = spi_sync_transfer(spi, xfers, 2);
>  		if (rc < 0) {
> -- 
> 2.25.1
>