| 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
| ||
|
Message-ID: <BN7PR12MB280270590FF8B7DF27E07760DC799@BN7PR12MB2802.namprd12.prod.outlook.com>
Date: Tue, 16 May 2023 12:07:22 +0000
From: "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>,
Mark Brown <broonie@...nel.org>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
NXP Linux Team <linux-imx@....com>
CC: Marc Kleine-Budde <mkl@...gutronix.de>,
"linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"git (AMD-Xilinx)" <git@....com>
Subject: RE: [PATCH 3/3] spi: spi-imx: fix use of more than four chipselects
Hello,
> -----Original Message-----
> From: Rasmus Villemoes <linux@...musvillemoes.dk>
> Sent: Tuesday, April 25, 2023 7:15 PM
> To: Mark Brown <broonie@...nel.org>; Shawn Guo <shawnguo@...nel.org>;
> Sascha Hauer <s.hauer@...gutronix.de>; Pengutronix Kernel Team
> <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>; NXP Linux
> Team <linux-imx@....com>
> Cc: Marc Kleine-Budde <mkl@...gutronix.de>; Rasmus Villemoes
> <linux@...musvillemoes.dk>; linux-spi@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: [PATCH 3/3] spi: spi-imx: fix use of more than four chipselects
>
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
>
>
> Currently, the spi->chip_select is used unconditionally in code such as
>
> /* set chip select to use */
> ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
>
> and
>
> if (spi->mode & SPI_CPHA)
> cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> else
> cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
>
> with these macros being
>
> #define MX51_ECSPI_CTRL_CS(cs) ((cs) << 18)
> #define MX51_ECSPI_CONFIG_SCLKPHA(cs) (1 << ((cs) + 0))
>
> However, the CHANNEL_SELECT field in the control register is only two bits
> wide, so when spi->chip_select >= 4, we end up writing garbage into the
> BURST_LENGTH field. Similarly, there are only four bits in the SCLK_PHA field,
> so the code above ends up actually modifying bits in the SCLK_POL (or higher)
> field.
>
> The scrambling of the BURST_LENGTH field itself is probably benign, since
> that is explicitly completely initialized later, in
> ->prepare_transfer.
>
> But, since we effectively write (spi->chip_select & 3) into the
> CHANNEL_SELECT field, that value is what the IP block then uses to determine
> which bits of the configuration register control phase, polarity etc., and those
> bits are not properly initialized, so communication with the spi device
> completely fails.
>
> Fix this by using the ->unused_native_cs value as channel number for any spi
> device which uses a gpio as chip select.
>
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> ---
> drivers/spi/spi-imx.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> e8f7afbd9847..569a5132f324 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -504,6 +504,13 @@ static void mx51_ecspi_disable(struct spi_imx_data
> *spi_imx)
> writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL); }
>
> +static int mx51_ecspi_channel(const struct spi_device *spi) {
> + if (!spi->cs_gpiod)
> + return spi->chip_select;
New set/get APIs for accessing spi->chip_select and spi->cs_gpiod were introduced by
https://github.com/torvalds/linux/commit/303feb3cc06ac0665d0ee9c1414941200e60e8a3
please use these APIs instead of accessing spi->chip_select & spi->cs_gpiod directly.
Regards,
Amit
> + return spi->controller->unused_native_cs;
> +}
> +
> static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
> struct spi_message *msg) { @@ -514,6 +521,7 @@
> static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
> u32 testreg, delay;
> u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> u32 current_cfg = cfg;
> + int channel = mx51_ecspi_channel(spi);
>
> /* set Master or Slave mode */
> if (spi_imx->slave_mode)
> @@ -528,7 +536,7 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
> ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);
>
> /* set chip select to use */
> - ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
> + ctrl |= MX51_ECSPI_CTRL_CS(channel);
>
> /*
> * The ctrl register must be written first, with the EN bit set other @@ -
> 549,22 +557,22 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
> * BURST_LENGTH + 1 bits are received
> */
> if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> - cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
> + cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(channel);
> else
> - cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
> + cfg |= MX51_ECSPI_CONFIG_SBBCTRL(channel);
>
> if (spi->mode & SPI_CPOL) {
> - cfg |= MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
> - cfg |= MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
> + cfg |= MX51_ECSPI_CONFIG_SCLKPOL(channel);
> + cfg |= MX51_ECSPI_CONFIG_SCLKCTL(channel);
> } else {
> - cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
> - cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
> + cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(channel);
> + cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(channel);
> }
>
> if (spi->mode & SPI_CS_HIGH)
> - cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
> + cfg |= MX51_ECSPI_CONFIG_SSBPOL(channel);
> else
> - cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
> + cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(channel);
>
> if (cfg == current_cfg)
> return 0;
> @@ -609,14 +617,15 @@ static void mx51_configure_cpha(struct
> spi_imx_data *spi_imx,
> bool cpha = (spi->mode & SPI_CPHA);
> bool flip_cpha = (spi->mode & SPI_RX_CPHA_FLIP) && spi_imx->rx_only;
> u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> + int channel = mx51_ecspi_channel(spi);
>
> /* Flip cpha logical value iff flip_cpha */
> cpha ^= flip_cpha;
>
> if (cpha)
> - cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> + cfg |= MX51_ECSPI_CONFIG_SCLKPHA(channel);
> else
> - cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> + cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(channel);
>
> writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG); }
> --
> 2.37.2
Powered by blists - more mailing lists