Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 11:35:51 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Dhruva Gole <d-gole@ti.com>
Cc:     Nathan Barrett-Morrison <nathan.morrison@timesys.com>,
        greg.malysa@timesys.com,
        "open list:SPI SUBSYSTEM" <linux-spi@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] spi: cadence-quadspi: Add upper limit safety check to
 baudrate divisor
Message-ID: <Y39XFzYJL3EmxSFF@sirena.org.uk>
References: <20221123211705.126900-1-nathan.morrison@timesys.com>
 <9e5264fa-db1a-ed96-5fd8-cbfa4694b8bd@ti.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="lnq7tpudOIivlUPp"
Content-Disposition: inline
In-Reply-To: <9e5264fa-db1a-ed96-5fd8-cbfa4694b8bd@ti.com>
Precedence: bulk


--lnq7tpudOIivlUPp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Nov 24, 2022 at 12:16:10PM +0530, Dhruva Gole wrote:
> On 24/11/22 02:47, Nathan Barrett-Morrison wrote:

> > +	/* Maximum baud divisor */
> > +	if (div > CQSPI_REG_CONFIG_BAUD_MASK)

> I don't think comparing "greater than" with a MASK is atall a good idea.

Why - it's checking that the calculated divisor can actually fit in the
relevant register field which seems like a totally normal thing to do?

> Again, I don't fully understand your situation is as in
> what is the peripheral you are using. So please elaborate on that.

As far as I can tell the issue here is that the device is asking for a
rate which requires a larger divisor than the controller can support but
the driver doesn't do any bounds checking so it just writes the
calculated divisor out to the hardware, corrupting any adjacent fields.

In this context the SPI controller is a peripheral within the SoC.

> Importantly, I would suggest that you _NEVER_ compare ANY value to a
> MASK Macro. MASK Macros are meant to MASK bits.

It's very common to also use masks to identify when values have
overflowed the values that can be written to a field.

--lnq7tpudOIivlUPp
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmN/VxcACgkQJNaLcl1U
h9BMpgf/cLsAMTa2AU1EcDgf6Rm2h9wt/E0cD5OwpVV+MrTVABVz3uKgDoNYwfxE
ka2++YyWBIzhbbKtdnvYhBmyeipb+uHTuGkx5+MCZT37sVjJhM3OB87Lja3xzBMD
bzpgTL5D00f4AWPwB3sI4QaU8IzjiaBWZ0vSEaSpMDHHq6y2miWn1q53JcluAZ3A
+ihJvhoFI3c8dO3PfXG8M8nnqD92X9HQpd0URp/4wAszq4RJTqyvwXZCD+Y0ECUw
ES24GzTtJbRbASi9id6DqtaqO1kE6DT3nS/pIjIqSixRHjqPqySeWJDz93XU/KhU
e04SV9UMdIcA5vhAaHDqKE9rHfw02A==
=P2Sd
-----END PGP SIGNATURE-----

--lnq7tpudOIivlUPp--