Re: [PATCH] sc16is7xx: null ptr check

Re: [PATCH] sc16is7xx: null ptr check

[Uwe Kleine-König]

[fix quoting, adding broonie and linux-spi to addressees]

On Mon, Oct 05, 2015 at 07:43:37AM +0200, Sean Nyekjær wrote:
> On 2015-10-04 22:23, Uwe Kleine-König wrote:
> >On Mon, Aug 31, 2015 at 12:01:27PM +0200, Sean Nyekjaer wrote:
> >>If a wrong compatible flag in specified, the of_match_device
> >>returning null.
> >>Implemented check and if NULL then returning -ENODEV.
> >>
> >>Signed-off-by: Sean Nyekjaer <sean.nyekjaer-rjjw5hvvQKZaa/9Udqfwiw@public.gmane.org>
> >>---
> >>  drivers/tty/serial/sc16is7xx.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >>diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >>index 72ffd0d..2fce9fb 100644
> >>--- a/drivers/tty/serial/sc16is7xx.c
> >>+++ b/drivers/tty/serial/sc16is7xx.c
> >>@@ -1320,6 +1320,8 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
> >>  	if (spi->dev.of_node) {
> >>  		const struct of_device_id *of_id =
> >>  			of_match_device(sc16is7xx_dt_ids, &spi->dev);
> >>+	if (!of_id)
> >>+		return -ENODEV;
> >
> >Did you see a problem here? I wonder if this can ever trigger. In the
>
> Yes i did :-)
>
> >commit log you wrote "If a wrong compatible flag in specified, ...", but
> >then the probe function isn't called at all, is it?
> >
> >>  		devtype = (struct sc16is7xx_devtype *)of_id->data;
> >>  	} else {
> 
> Here you can see code snippet of the dtb that triggered the NULL
> pointer dereference:
>     sc16is750@1 {
>         compatible = "sc16is750";
>         spi-max-frequency = <2000000>;
>         reg = <1>;
>         clocks = <&clk14m>;
>         interrupt-parent = <&gpio7>;
>         interrupts = <5 2>;
>     }
> 
> And what my dtb should have looked like:
>     sc16is750@1 {
>         compatible = "nxp,sc16is750";
>         spi-max-frequency = <2000000>;
>         reg = <1>;
>         clocks = <&clk14m>;
>         interrupt-parent = <&gpio7>;
>         interrupts = <5 2>;
>     }

The relevant driver details are:

static const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
	...
	{ .compatible = "nxp,sc16is750",        .data = &sc16is750_devtype, },
	...
	{ }
};

...

static const struct spi_device_id sc16is7xx_spi_id_table[] = {
	...
	{ "sc16is750",  (kernel_ulong_t)&sc16is750_devtype, },
	...
	{ }
};

So in the broken case the device in question was bound to the
sc16is7xx driver because the compatible name was matched by the
id_table and not the of_match_table. My first thought was that in this
case the spi_device's dev->of_node should not be set, but after having
thought a bit more I think this is right.

But the .probe routine could be a tad more clever here:

	devtype = of_device_get_match_data(&spi->dev);
	if (!devtype) {
		const struct spi_device_id *id_entry = spi_get_device_id(spi);

		/* Do I need to check id_entry being != NULL here? */
		devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
		flags = IRQF_TRIGGER_FALLING;
	}

Apart from that using "flags = IRQF_TRIGGER_FALLING" iff the device was
probed via spi_id_table is strange.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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] sc16is7xx: null ptr check

[Mark Brown]

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

On Mon, Oct 05, 2015 at 08:31:28AM +0200, Uwe Kleine-König wrote:

> So in the broken case the device in question was bound to the
> sc16is7xx driver because the compatible name was matched by the
> id_table and not the of_match_table. My first thought was that in this
> case the spi_device's dev->of_node should not be set, but after having
> thought a bit more I think this is right.

Yes, it's intended behaviour - the DT is buggy here.

> But the .probe routine could be a tad more clever here:

> 	devtype = of_device_get_match_data(&spi->dev);
> 	if (!devtype) {
> 		const struct spi_device_id *id_entry = spi_get_device_id(spi);
> 
> 		/* Do I need to check id_entry being != NULL here? */
> 		devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
> 		flags = IRQF_TRIGGER_FALLING;
> 	}

That's pretty much a requirement - the driver should work no matter
which binding method gets used, OF only drivers aren't great practice at
the best of times and for off-SoC devices like this one it's definitely
not appropriate.

> Apart from that using "flags = IRQF_TRIGGER_FALLING" iff the device was
> probed via spi_id_table is strange.

That seems like an obvious bug.

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