Re: [PATCH v2] driver: iio: add missing checks on iio_info's callback access

From: Jonathan Cameron
Date: Wed May 29 2024 - 10:13:32 EST

On Wed, 29 May 2024 15:10:42 +0200
Julien Stephan <jstephan@xxxxxxxxxxxx> wrote:

> Le mer. 29 mai 2024 à 14:05, Jonathan Cameron
> <Jonathan.Cameron@xxxxxxxxxx> a écrit :
> >
> > On Wed, 29 May 2024 13:55:52 +0200
> > Julien Stephan <jstephan@xxxxxxxxxxxx> wrote:
> >
> > > Some callbacks from iio_info structure are accessed without any check, so
> > > if a driver doesn't implement them trying to access the corresponding
> > > sysfs entries produce a kernel oops such as:
> > >
> > > [ 2203.527791] Unable to handle kernel NULL pointer dereference at virtual address 00000000 when execute
> > > [...]
> > > [ 2203.783416] Call trace:
> > > [ 2203.783429] iio_read_channel_info_avail from dev_attr_show+0x18/0x48
> > > [ 2203.789807] dev_attr_show from sysfs_kf_seq_show+0x90/0x120
> > > [ 2203.794181] sysfs_kf_seq_show from seq_read_iter+0xd0/0x4e4
> > > [ 2203.798555] seq_read_iter from vfs_read+0x238/0x2a0
> > > [ 2203.802236] vfs_read from ksys_read+0xa4/0xd4
> > > [ 2203.805385] ksys_read from ret_fast_syscall+0x0/0x54
> > > [ 2203.809135] Exception stack(0xe0badfa8 to 0xe0badff0)
> > > [ 2203.812880] dfa0: 00000003 b6f10f80 00000003 b6eab000 00020000 00000000
> > > [ 2203.819746] dfc0: 00000003 b6f10f80 7ff00000 00000003 00000003 00000000 00020000 00000000
> > > [ 2203.826619] dfe0: b6e1bc88 bed80958 b6e1bc94 b6e1bcb0
> > > [ 2203.830363] Code: bad PC value
> > > [ 2203.832695] ---[ end trace 0000000000000000 ]---
> > >
> > > Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
> >
> > How bad would a registration time check look?
> > I'd rather catch this early than have drivers with missing hooks
> > that we don't notice because no one pokes the file.
>
> Hi Jonathan,
>
> Do you mean something like that (as it is done for ext_info for example) :
>
> ret = __iio_add_chan_devattr(iio_chan_info_postfix[i],
> chan,
> - &iio_read_channel_info,
> - &iio_write_channel_info,
> + indio_dev->info->read_raw ?
> + &iio_read_channel_info : NULL,

Doesn't work because of the read_raw_multi callback, but otherwise
this does improve our permissions handling a little at least.
It 'might' be considered an ABI change though :(

> + indio_dev->info->write_raw ?
> + &iio_write_channel_info : NULL,
> i,
> shared_by,
> &indio_dev->dev,
> NULL,
> &iio_dev_opaque->channel_attr_list);
>
> Or do you want to check even before and do not create the sysfs
> entry if there is no callback registered by the driver?

I was thinking a much more stupid option of a missing read_raw
and read_raw_multi + anything in the info_masks pretty much
indicates a bug.

I don't think we have any 'write only' attributes

Similar for read_event_config, though write_event_config is
trickier as we 'might' one day have a device where the events
are all fixed value and always on (so read only).

Perhaps what you have here is the simplest option as the exact
rules for what callbacks are provided area bit messy so checking
at use is fine.

However I'd like to see some scattered use of local variables like
in inkern.c
struct iio_info *info = chan->indio_dev->info;
to reduce the long lines.


>
> Julien
>
> >
> > The inkern ones are good though.
> >
> > Jonathan
> >
> > > ---
> > > Changes in v2:
> > > - crop dmesg log to show only pertinent info and reduce commit message
> > > - Link to v1: https://lore.kernel.org/r/20240529-iio-core-fix-segfault-v1-1-7ff1ba881d38@xxxxxxxxxxxx
> > > ---
> > > drivers/iio/industrialio-core.c | 7 ++++++-
> > > drivers/iio/industrialio-event.c | 9 +++++++++
> > > drivers/iio/inkern.c | 16 +++++++++++-----
> > > 3 files changed, 26 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index fa7cc051b4c4..2f185b386949 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -758,9 +758,11 @@ static ssize_t iio_read_channel_info(struct device *dev,
> > > INDIO_MAX_RAW_ELEMENTS,
> > > vals, &val_len,
> > > this_attr->address);
> > > - else
> > > + else if (indio_dev->info->read_raw)
> > > ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
> > > &vals[0], &vals[1], this_attr->address);
> > > + else
> > > + return -EINVAL;
> > >
> > > if (ret < 0)
> > > return ret;
> > > @@ -842,6 +844,9 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
> > > int length;
> > > int type;
> > >
> > > + if (!indio_dev->info->read_avail)
> > > + return -EINVAL;
> > > +
> > > ret = indio_dev->info->read_avail(indio_dev, this_attr->c,
> > > &vals, &type, &length,
> > > this_attr->address);
> > > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> > > index 910c1f14abd5..a64f8fbac597 100644
> > > --- a/drivers/iio/industrialio-event.c
> > > +++ b/drivers/iio/industrialio-event.c
> > > @@ -285,6 +285,9 @@ static ssize_t iio_ev_state_store(struct device *dev,
> > > if (ret < 0)
> > > return ret;
> > >
> > > + if (!indio_dev->info->write_event_config)
> > > + return -EINVAL;
> > > +
> > > ret = indio_dev->info->write_event_config(indio_dev,
> > > this_attr->c, iio_ev_attr_type(this_attr),
> > > iio_ev_attr_dir(this_attr), val);
> > > @@ -300,6 +303,9 @@ static ssize_t iio_ev_state_show(struct device *dev,
> > > struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > int val;
> > >
> > > + if (!indio_dev->info->read_event_config)
> > > + return -EINVAL;
> > > +
> > > val = indio_dev->info->read_event_config(indio_dev,
> > > this_attr->c, iio_ev_attr_type(this_attr),
> > > iio_ev_attr_dir(this_attr));
> > > @@ -318,6 +324,9 @@ static ssize_t iio_ev_value_show(struct device *dev,
> > > int val, val2, val_arr[2];
> > > int ret;
> > >
> > > + if (!indio_dev->info->read_event_value)
> > > + return -EINVAL;
> > > +
> > > ret = indio_dev->info->read_event_value(indio_dev,
> > > this_attr->c, iio_ev_attr_type(this_attr),
> > > iio_ev_attr_dir(this_attr), iio_ev_attr_info(this_attr),
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index 52d773261828..74f87f6ac390 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -560,9 +560,11 @@ static int iio_channel_read(struct iio_channel *chan, int *val, int *val2,
> > > vals, &val_len, info);
> > > *val = vals[0];
> > > *val2 = vals[1];
> > > - } else {
> > > + } else if (chan->indio_dev->info->read_raw) {
> > > ret = chan->indio_dev->info->read_raw(chan->indio_dev,
> > > chan->channel, val, val2, info);
> > > + } else {
> > > + return -EINVAL;
> > > }
> > >
> > > return ret;
> > > @@ -753,8 +755,10 @@ static int iio_channel_read_avail(struct iio_channel *chan,
> > > if (!iio_channel_has_available(chan->channel, info))
> > > return -EINVAL;
> > >
> > > - return chan->indio_dev->info->read_avail(chan->indio_dev, chan->channel,
> > > - vals, type, length, info);
> > > + if (chan->indio_dev->info->read_avail)
> > > + return chan->indio_dev->info->read_avail(chan->indio_dev, chan->channel,
> > > + vals, type, length, info);
> > > + return -EINVAL;
> > > }
> > >
> > > int iio_read_avail_channel_attribute(struct iio_channel *chan,
> > > @@ -917,8 +921,10 @@ EXPORT_SYMBOL_GPL(iio_get_channel_type);
> > > static int iio_channel_write(struct iio_channel *chan, int val, int val2,
> > > enum iio_chan_info_enum info)
> > > {
> > > - return chan->indio_dev->info->write_raw(chan->indio_dev,
> > > - chan->channel, val, val2, info);
> > > + if (chan->indio_dev->info->write_raw)
> > > + return chan->indio_dev->info->write_raw(chan->indio_dev,
> > > + chan->channel, val, val2, info);
> > > + return -EINVAL;
> > > }
> > >
> > > int iio_write_channel_attribute(struct iio_channel *chan, int val, int val2,
> > >
> > > ---
> > > base-commit: 409b6d632f5078f3ae1018b6e43c32f2e12f6736
> > > change-id: 20240528-iio-core-fix-segfault-aa74be7eee4a
> > >
> > > Best regards,
> >