[PATCH] iio: adxl345: move null check for i2c id at start of probe

[PATCH] iio: adxl345: move null check for i2c id at start of probe

[Alexandru Ardelean]

Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").

This was found via static checker.
After looking into the code a bit, it's unlikely that there will be a NULL
dereference if the `id` object in that specific code path.
However, it's safe to add a NULL (paranoid) check just to make sure and
remove any uncertainties.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/adxl345_i2c.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 785c89de91e7..f22f71315a0c 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,6 +27,9 @@ static int adxl345_i2c_probe(struct i2c_client *client,
 {
 	struct regmap *regmap;
 
+	if (!id)
+		return -ENODEV;
+
 	regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(&client->dev, "Error initializing i2c regmap: %ld\n",
@@ -35,7 +38,7 @@ static int adxl345_i2c_probe(struct i2c_client *client,
 	}
 
 	return adxl345_core_probe(&client->dev, regmap, id->driver_data,
-				  id ? id->name : NULL);
+				  id->name);
 }
 
 static int adxl345_i2c_remove(struct i2c_client *client)
-- 
2.17.1

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Himanshu Jha]

On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:
> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
> 
> This was found via static checker.
> After looking into the code a bit, it's unlikely that there will be a NULL
> dereference if the `id` object in that specific code path.
> However, it's safe to add a NULL (paranoid) check just to make sure and
> remove any uncertainties.

I would like to know when would that case happen actually ?

Because probe will only be called only when a match occurs either
through DT or id matching. Isn't it ?

-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Jonathan Cameron]

On Sat, 11 Aug 2018 15:48:33 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:
> > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
> > 
> > This was found via static checker.
> > After looking into the code a bit, it's unlikely that there will be a NULL
> > dereference if the `id` object in that specific code path.
> > However, it's safe to add a NULL (paranoid) check just to make sure and
> > remove any uncertainties.  
> 
> I would like to know when would that case happen actually ?
> 
> Because probe will only be called only when a match occurs either
> through DT or id matching. Isn't it ?
> 
Yes. Alternative would have just not been to check it, but this is fine
so applied. I'm not going to rush this through stable though given
I don't think it can actually happen.

Jonathan

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Himanshu Jha]

On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote:
> On Sat, 11 Aug 2018 15:48:33 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:
> > > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
> > > 
> > > This was found via static checker.
> > > After looking into the code a bit, it's unlikely that there will be a NULL
> > > dereference if the `id` object in that specific code path.
> > > However, it's safe to add a NULL (paranoid) check just to make sure and
> > > remove any uncertainties.  
> > 
> > I would like to know when would that case happen actually ?
> > 
> > Because probe will only be called only when a match occurs either
> > through DT or id matching. Isn't it ?
> > 
> Yes. Alternative would have just not been to check it, but this is fine
> so applied. I'm not going to rush this through stable though given
> I don't think it can actually happen.

Thanks for the confirmation.

So, I have another doubt and it seems to be right time to ask.

BME680 currently supports both ACPI matching and traditional ID
matching. So, it there any priority list to which patch the driver
would choose to match the device.

ACPI > ID matching ? (In my case this happens)

Because this matching tends to decide the `name` attribute of loaded
driver.

For ACPI: BME0680 (not sure maybe it was I2C0:BME0680)
For ID: bme680

-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Lars-Peter Clausen]

On 08/19/2018 07:43 PM, Himanshu Jha wrote:
> On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote:
>> On Sat, 11 Aug 2018 15:48:33 +0530
>> Himanshu Jha <himanshujha199640@gmail.com> wrote:
>>
>>> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:
>>>> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
>>>>
>>>> This was found via static checker.
>>>> After looking into the code a bit, it's unlikely that there will be a NULL
>>>> dereference if the `id` object in that specific code path.
>>>> However, it's safe to add a NULL (paranoid) check just to make sure and
>>>> remove any uncertainties.  
>>>
>>> I would like to know when would that case happen actually ?
>>>
>>> Because probe will only be called only when a match occurs either
>>> through DT or id matching. Isn't it ?
>>>
>> Yes. Alternative would have just not been to check it, but this is fine
>> so applied. I'm not going to rush this through stable though given
>> I don't think it can actually happen.
> 
> Thanks for the confirmation.
> 
> So, I have another doubt and it seems to be right time to ask.
> 
> BME680 currently supports both ACPI matching and traditional ID
> matching. So, it there any priority list to which patch the driver
> would choose to match the device.
> 
> ACPI > ID matching ? (In my case this happens)
> 
> Because this matching tends to decide the `name` attribute of loaded
> driver.
> 
> For ACPI: BME0680 (not sure maybe it was I2C0:BME0680)
> For ID: bme680

Yeah, that's wrong. But pretty much all ACPI drivers have that issue.
Maybe we should just deprecate the name attribute.

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Himanshu Jha]

On Sun, Aug 19, 2018 at 07:59:38PM +0200, Lars-Peter Clausen wrote:
> On 08/19/2018 07:43 PM, Himanshu Jha wrote:
> > On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote:
> >> On Sat, 11 Aug 2018 15:48:33 +0530
> >> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> >>
> >>> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:
> >>>> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
> >>>>
> >>>> This was found via static checker.
> >>>> After looking into the code a bit, it's unlikely that there will be a NULL
> >>>> dereference if the `id` object in that specific code path.
> >>>> However, it's safe to add a NULL (paranoid) check just to make sure and
> >>>> remove any uncertainties.  
> >>>
> >>> I would like to know when would that case happen actually ?
> >>>
> >>> Because probe will only be called only when a match occurs either
> >>> through DT or id matching. Isn't it ?
> >>>
> >> Yes. Alternative would have just not been to check it, but this is fine
> >> so applied. I'm not going to rush this through stable though given
> >> I don't think it can actually happen.
> > 
> > Thanks for the confirmation.
> > 
> > So, I have another doubt and it seems to be right time to ask.
> > 
> > BME680 currently supports both ACPI matching and traditional ID
> > matching. So, it there any priority list to which patch the driver
> > would choose to match the device.
> > 
> > ACPI > ID matching ? (In my case this happens)
> > 
> > Because this matching tends to decide the `name` attribute of loaded
> > driver.
> > 
> > For ACPI: BME0680 (not sure maybe it was I2C0:BME0680)
> > For ID: bme680
> 
> Yeah, that's wrong. But pretty much all ACPI drivers have that issue.
> Maybe we should just deprecate the name attribute.

libiio is the most affected due to this issue as I can figure out.
Particularly, the iio_device_get_name() api:
https://analogdevicesinc.github.io/libiio/group__Context.html#gae5807303b638869679ece67270e72e77

-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Jonathan Cameron]

On Mon, 20 Aug 2018 00:24:58 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Sun, Aug 19, 2018 at 07:59:38PM +0200, Lars-Peter Clausen wrote:
> > On 08/19/2018 07:43 PM, Himanshu Jha wrote:  
> > > On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote:  
> > >> On Sat, 11 Aug 2018 15:48:33 +0530
> > >> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> > >>  
> > >>> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:  
> > >>>> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
> > >>>>
> > >>>> This was found via static checker.
> > >>>> After looking into the code a bit, it's unlikely that there will be a NULL
> > >>>> dereference if the `id` object in that specific code path.
> > >>>> However, it's safe to add a NULL (paranoid) check just to make sure and
> > >>>> remove any uncertainties.    
> > >>>
> > >>> I would like to know when would that case happen actually ?
> > >>>
> > >>> Because probe will only be called only when a match occurs either
> > >>> through DT or id matching. Isn't it ?
> > >>>  
> > >> Yes. Alternative would have just not been to check it, but this is fine
> > >> so applied. I'm not going to rush this through stable though given
> > >> I don't think it can actually happen.  
> > > 
> > > Thanks for the confirmation.
> > > 
> > > So, I have another doubt and it seems to be right time to ask.
> > > 
> > > BME680 currently supports both ACPI matching and traditional ID
> > > matching. So, it there any priority list to which patch the driver
> > > would choose to match the device.
> > > 
> > > ACPI > ID matching ? (In my case this happens)
> > > 
> > > Because this matching tends to decide the `name` attribute of loaded
> > > driver.
> > > 
> > > For ACPI: BME0680 (not sure maybe it was I2C0:BME0680)
> > > For ID: bme680  
> > 
> > Yeah, that's wrong. But pretty much all ACPI drivers have that issue.
> > Maybe we should just deprecate the name attribute.  
> 
> libiio is the most affected due to this issue as I can figure out.
> Particularly, the iio_device_get_name() api:
> https://analogdevicesinc.github.io/libiio/group__Context.html#gae5807303b638869679ece67270e72e77
> 

Yeah, the name thing was one of those ones that got away from us a long
time ago and became ABI in far too many drivers.  The 'intent' was
that it would just provide a convenient "what's this part?" sysfs
attribute, so should always return the part number rather than anything
to do with any of the bindings.  Unfortunately it often doesn't.

We could add a new attribute called something like 'part_number'
as then it should be more obvious what the intent is an hopefully it'll
get used right.

Jonathan

Re: [PATCH] iio: adxl345: move null check for i2c id at start of probe

[Dan Carpenter]

On Sat, Aug 11, 2018 at 03:48:33PM +0530, Himanshu Jha wrote:
> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote:
> > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375").
> > 
> > This was found via static checker.
> > After looking into the code a bit, it's unlikely that there will be a NULL
> > dereference if the `id` object in that specific code path.
> > However, it's safe to add a NULL (paranoid) check just to make sure and
> > remove any uncertainties.
> 
> I would like to know when would that case happen actually ?
> 
> Because probe will only be called only when a match occurs either
> through DT or id matching. Isn't it ?
> 

Btw, normally Smatch doesn't warn about inconsistent NULL checking if
it's clear that the NULL check is not required.  In this case, the
adxl345 is not in my allmodconfig so I didn't build the cross function
DB for it so I didn't have the caller information.  And also:

drivers/i2c/i2c-core-base.c
   391          /*
   392           * When there are no more users of probe(),
   393           * rename probe_new to probe.
   394           */
   395          if (driver->probe_new)
   396                  status = driver->probe_new(client);
   397          else if (driver->probe)
   398                  status = driver->probe(client,
   399                                         i2c_match_id(driver->id_table, client));
   400          else
   401                  status = -EINVAL;
   402  

Smatch isn't smart enough to know that i2c_match_id() doesn't return a
NULL.  I did look at the code before sending the bug report but it
wasn't immediately clear to me either whether it was possible or not.

regards,
dan carpenter