Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Milan Zamazal <mzamazal@redhat.com>
To:     Nuno =?utf-8?Q?S=C3=A1?= <noname.nuno@gmail.com>
Cc:     linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH] iio: core: Prevent invalid memory access when there is
 no parent
References: <20230718120700.132579-1-mzamazal@redhat.com>
        <4a5d6ca0e2c9529d5eb0bbacbaed56e2645719c6.camel@gmail.com>
Date:   Tue, 18 Jul 2023 16:35:18 +0200
In-Reply-To: <4a5d6ca0e2c9529d5eb0bbacbaed56e2645719c6.camel@gmail.com>
 ("Nuno
        =?utf-8?Q?S=C3=A1=22's?= message of "Tue, 18 Jul 2023 15:10:07 +0200")
Message-ID: <87lefdcmop.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Nuno S=C3=A1 <noname.nuno@gmail.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.=C2=A0 Probably an unwanted change in t=
hat
>> commit was that if the device has no parent then NULL pointer is
>> accessed.=C2=A0 This is what happens in the stock IIO dummy driver when a
>> new entry is created in configfs:
>>=20
>> =C2=A0 # mkdir /sys/kernel/config/iio/devices/dummy/foo
>> =C2=A0 BUG: kernel NULL pointer dereference, address: 0000000000000278
>> =C2=A0 ...
>> =C2=A0 ? asm_exc_page_fault+0x22/0x30
>> =C2=A0 ? container_offline+0x20/0x20
>> =C2=A0 __iio_device_register+0x45/0xc10
>> =C2=A0 ? krealloc+0x73/0xa0
>> =C2=A0 ? iio_device_attach_buffer+0x31/0xc0
>> =C2=A0 ? iio_simple_dummy_configure_buffer+0x20/0x20
>> =C2=A0 ? iio_triggered_buffer_setup_ext+0xb4/0x100
>> =C2=A0 iio_dummy_probe+0x112/0x190
>> =C2=A0 iio_sw_device_create+0xa8/0xd0
>> =C2=A0 device_make_group+0xe/0x40
>> =C2=A0 configfs_mkdir+0x1a6/0x440
>>=20
>> Since there seems to be no reason to make a parent device of an IIO
>> dummy device mandatory, let=E2=80=99s prevent the invalid memory access =
in
>> __iio_device_register when the parent device is NULL.=C2=A0 With this
>> change, the IIO dummy driver works fine with configfs.
>>=20
>> Fixes: 813665564b3d ("iio: core: Convert to use firmware node handle ins=
tead
>> of OF node")
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>
> LGTM (just one minor question below...)
>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>
>> =C2=A0drivers/iio/industrialio-core.c | 11 ++++++-----
>> =C2=A01 file changed, 6 insertions(+), 5 deletions(-)
>>=20
>> 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;
>> =C2=A0int __iio_device_register(struct iio_dev *indio_dev, struct module=
 *this_mod)
>> =C2=A0{
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct iio_dev_opaque *i=
io_dev_opaque =3D to_iio_dev_opaque(indio_dev);
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct fwnode_handle *fwnode;
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct fwnode_handle *fwnode =
=3D NULL;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret;
>> =C2=A0
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!indio_dev->info)
>> @@ -1899,11 +1899,12 @@ int __iio_device_register(struct iio_dev *indio_=
dev,
>> struct module *this_mod)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* If the calling driver=
 did not initialize firmware node, do it here
>> */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (dev_fwnode(&indio_de=
v->dev))
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0fwnode =3D dev_fwnode(&indio_dev->dev);
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0else
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0else if (indio_dev->dev.paren=
t !=3D NULL)
>
> Maybe this is odd enough that it could have an explicit comment referenci=
ng the
> iio-dummy device?=20

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=C3=A1