linux-kernel - Re: [PATCH] iio: core: Prevent invalid memory access when there is no parent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87lefdcmop.fsf@redhat.com>
Date:   Tue, 18 Jul 2023 16:35:18 +0200
From:   Milan Zamazal <mzamazal@...hat.com>
To:     Nuno Sá <noname.nuno@...il.com>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] iio: core: Prevent invalid memory access when there is
 no parent

Nuno Sá <noname.nuno@...il.com> writes:

> On Tue, 2023-07-18 at 14:07 +0200, Milan Zamazal wrote:
>> Commit 813665564b3d ("iio: core: Convert to use firmware node handle
>> instead of OF node") switched the kind of nodes to use for label
>
>> retrieval in device registration.  Probably an unwanted change in that
>> commit was that if the device has no parent then NULL pointer is
>> accessed.  This is what happens in the stock IIO dummy driver when a
>> new entry is created in configfs:
>> 
>>   # mkdir /sys/kernel/config/iio/devices/dummy/foo
>>   BUG: kernel NULL pointer dereference, address: 0000000000000278
>>   ...
>>   ? asm_exc_page_fault+0x22/0x30
>>   ? container_offline+0x20/0x20
>>   __iio_device_register+0x45/0xc10
>>   ? krealloc+0x73/0xa0
>>   ? iio_device_attach_buffer+0x31/0xc0
>>   ? iio_simple_dummy_configure_buffer+0x20/0x20
>>   ? iio_triggered_buffer_setup_ext+0xb4/0x100
>>   iio_dummy_probe+0x112/0x190
>>   iio_sw_device_create+0xa8/0xd0
>>   device_make_group+0xe/0x40
>>   configfs_mkdir+0x1a6/0x440
>> 
>> Since there seems to be no reason to make a parent device of an IIO
>> dummy device mandatory, let’s prevent the invalid memory access in
>> __iio_device_register when the parent device is NULL.  With this
>> change, the IIO dummy driver works fine with configfs.
>> 
>> Fixes: 813665564b3d ("iio: core: Convert to use firmware node handle instead
>> of OF node")
>> Signed-off-by: Milan Zamazal <mzamazal@...hat.com>
>> ---
>
> LGTM (just one minor question below...)
>
> Reviewed-by: Nuno Sa <nuno.sa@...log.com>
>
>>  drivers/iio/industrialio-core.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index c117f50d0cf3..229527b3434a 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1888,7 +1888,7 @@ static const struct iio_buffer_setup_ops
>> noop_ring_setup_ops;
>>  int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>>  {
>>         struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>> -       struct fwnode_handle *fwnode;
>> +       struct fwnode_handle *fwnode = NULL;
>>         int ret;
>>  
>>         if (!indio_dev->info)
>> @@ -1899,11 +1899,12 @@ int __iio_device_register(struct iio_dev *indio_dev,
>> struct module *this_mod)
>>         /* If the calling driver did not initialize firmware node, do it here
>> */
>>         if (dev_fwnode(&indio_dev->dev))
>>                 fwnode = dev_fwnode(&indio_dev->dev);
>> -       else
>> +       else if (indio_dev->dev.parent != NULL)
>
> Maybe this is odd enough that it could have an explicit comment referencing the
> iio-dummy device? 

Good idea, also to not forget about iio-dummy generally.  I'll add it.

> Not sure if there's any other place where this can actually happen...
>
> Apparently there are also some dev_err() on the parent device (even though
> dev_err() handles it) but yeah, unrelated with this.
>
>
> - Nuno Sá