spi: coldfire-qspi: Prevent NULL pointer dereference

If pdata->cs_control is NULL, we will hit NULL pointer dereference in mcfqspi_cs_select() and mcfqspi_cs_deselect(). Thus add NULL test for pdata->cs_control in probe().

---
 drivers/spi/spi-coldfire-qspi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index c29bbb0..b01616c 100644
@@ -133,13 +133,13 @@ static void mcfqspi_cs_deselect(struct mcfqspi *mcfqspi, u8 chip_select,
 
 static int mcfqspi_cs_setup(struct mcfqspi *mcfqspi)
 {
-	return (mcfqspi->cs_control && mcfqspi->cs_control->setup) ?
+	return (mcfqspi->cs_control->setup) ?
 		mcfqspi->cs_control->setup(mcfqspi->cs_control) : 0;
 }
 
 static void mcfqspi_cs_teardown(struct mcfqspi *mcfqspi)
 {
-	if (mcfqspi->cs_control && mcfqspi->cs_control->teardown)
+	if (mcfqspi->cs_control->teardown)
 		mcfqspi->cs_control->teardown(mcfqspi->cs_control);
 }
 

@@ -366,6 +366,11 @@ static int mcfqspi_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
+	if (!pdata->cs_control) {
+		dev_dbg(&pdev->dev, "pdata->cs_control is NULL\n");
+		return -EINVAL;
+	}
+
 	master = spi_alloc_master(&pdev->dev, sizeof(*mcfqspi));
 	if (master == NULL) {
 		dev_dbg(&pdev->dev, "spi_alloc_master failed\n");

1.8.1.2



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

NAK.

I suggest you read the code again. There is no NULL pointer dereference and
further, by returning an error if cs_control is NULL, you've now broken the design which was that cs_control is optional.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

So, you've sent this after I'd applied the patch (having ignored prior patches as well, the last activity was in 2012), that's a bit unfortunate. I've reverted the patch but please if you're reviewing changes to the driver can you indicate if things are OK when you do so - if patches aren't getting any review from people working on the driver the tendency is to assume that the driver has been abandoned.

D'oh! My bad. Actually you're right there would be a NULL pointer dereference in cs_select and cs_deselect (I was looking at the cs_setup and cs_teardown in the patch) and so cs_control must not be NULL. Thats want I get for replying before I've had my coffee and when I haven't looked at the code in awhile. So yeah, the patch is GTG.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I had the same first impression. But after looking through the code, the changes is fine. Furthermore, there's only on in-tree user it, which does provide cs_control.

There's more opportunity for cleanup, though, as cs_control provides control of a GPIO, which can be handled by the SPI core, too.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hrm, I managed to make the same mistake when I redid the verification somehow. Odd. Anyway, reinstated the patch.