[PATCH] spi-altera: Fix NULL pointer dereference in spi_bitbang_transfer_one

[PATCH] spi-altera: Fix NULL pointer dereference in spi_bitbang_transfer_one

[Iain Baron]

spi-altera.c is missing a bitbang setup_transfer callback to setup
the clock and wordsize for the SPI transfer.  This triggers a NULL
pointer dereference in spi_bitbang_transfer_one.
Provide the missing callback, which simply returns success, because
the parameters are not configurable at runtime for this hardware.

Signed-off-by: Iain Baron <iain.baron-YmHWzhjmega9Zq0HfjR8jw@public.gmane.org>
---
 drivers/spi/spi-altera.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index 595b62c..6b63f60 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -198,6 +198,12 @@ static irqreturn_t altera_spi_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static int altera_spi_setup_transfer(struct spi_device *spi,
+				struct spi_transfer *t)
+{
+	return 0;
+}
+
 static int altera_spi_probe(struct platform_device *pdev)
 {
 	struct altera_spi_platform_data *platp = dev_get_platdata(&pdev->dev);
@@ -224,6 +230,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 		return err;
 	hw->bitbang.chipselect = altera_spi_chipsel;
 	hw->bitbang.txrx_bufs = altera_spi_txrx;
+	hw->bitbang.setup_transfer = altera_spi_setup_transfer;
 
 	/* find and map our resources */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi-altera: Fix NULL pointer dereference in spi_bitbang_transfer_one

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On Wed, Dec 03, 2014 at 11:25:49AM +0000, Iain Baron wrote:
> spi-altera.c is missing a bitbang setup_transfer callback to setup
> the clock and wordsize for the SPI transfer.  This triggers a NULL
> pointer dereference in spi_bitbang_transfer_one.
> Provide the missing callback, which simply returns success, because
> the parameters are not configurable at runtime for this hardware.

If we can safely handle not having the callback then we should fix the
call site to safely handle a null pointer rather than adding dummy
callbacks.  That way this is fixed once for all drivers that need it.

I'd expect to see some code that checks to see if the caller is trying
to change paramters.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] spi-altera: Fix NULL pointer dereference in spi_bitbang_transfer_one

[Iain Baron]

> From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: 03 December 2014 13:03
> 
> If we can safely handle not having the callback then we should fix the
> call site to safely handle a null pointer rather than adding dummy
> callbacks.  That way this is fixed once for all drivers that need it.

Is it better to test for a null pointer at the spi-bitbang call-site, or get spi-bitbang to add a dummy callback of its own in spi_bitbang_start? The former has the overhead of a test every call, even for those drivers that provide the callback, while the latter has the overhead of a dummy call only for those drivers that don't.

At present, spi_bitbang_start assumes that if bitbang->txrx_bufs is defined, then bitbang->setup_transfer is also defined without checking it.

> I'd expect to see some code that checks to see if the caller is trying
> to change paramters.

If you mean in the spi-altera code: The spi-altera driver does not know what the hardware fixed parameters are, so it can't make a decision about whether the caller is trying to change them to something else.

If you mean in the spi-bitbang code: The code appears to do this with the do_setup test, but it will always do a setup for the very first transfer in a message regardless, invoking the missing callback.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi-altera: Fix NULL pointer dereference in spi_bitbang_transfer_one

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Wed, Dec 03, 2014 at 03:16:41PM +0000, Iain Baron wrote:

Please fix your mailer to word wrap within paragraphs so your mail is
legible.

> > If we can safely handle not having the callback then we should fix the
> > call site to safely handle a null pointer rather than adding dummy
> > callbacks.  That way this is fixed once for all drivers that need it.

> Is it better to test for a null pointer at the spi-bitbang call-site,
> or get spi-bitbang to add a dummy callback of its own in
> spi_bitbang_start? The former has the overhead of a test every call,
> even for those drivers that provide the callback, while the latter has
> the overhead of a dummy call only for those drivers that don't.

If this is a valid thing to do it is better to have the test; if you
really want to avoid the test then the core needs to provide the default
implementation that users can reference.

> > I'd expect to see some code that checks to see if the caller is trying
> > to change paramters.

> If you mean in the spi-altera code: The spi-altera driver does not
> know what the hardware fixed parameters are, so it can't make a
> decision about whether the caller is trying to change them to
> something else.

> If you mean in the spi-bitbang code: The code appears to do this with
> the do_setup test, but it will always do a setup for the very first
> transfer in a message regardless, invoking the missing callback.

In some generic code.  There's nothing device specific about a missing
callback.  If the device does not know the initial settings we can at
least check that nothing is trying to change the settings later.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]