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;
Message-ID: <4a5d6ca0e2c9529d5eb0bbacbaed56e2645719c6.camel@gmail.com>
Subject: Re: [PATCH] iio: core: Prevent invalid memory access when there is
 no parent
From:   Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To:     Milan Zamazal <mzamazal@redhat.com>, linux-iio@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:   Tue, 18 Jul 2023 15:10:07 +0200
In-Reply-To: <20230718120700.132579-1-mzamazal@redhat.com>
References: <20230718120700.132579-1-mzamazal@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 
MIME-Version: 1.0
Precedence: bulk

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 th=
at
> 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 i=
n
> __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 inst=
ead
> 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-c=
ore.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 *ii=
o_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_d=
ev,
> 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_dev=
->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.parent=
 !=3D NULL)

Maybe this is odd enough that it could have an explicit comment referencing=
 the
iio-dummy device? Not sure if there's any other place where this can actual=
ly
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