| 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
| ||
|
Message-ID: <20240602122940.1ac61c4d@jic23-huawei>
Date: Sun, 2 Jun 2024 12:29:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Julien Stephan <jstephan@...libre.com>, Lars-Peter Clausen
<lars@...afoo.de>, <linux-iio@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Nuno Sa <nuno.sa@...log.com>
Subject: Re: [PATCH v2] driver: iio: add missing checks on iio_info's
callback access
On Wed, 29 May 2024 15:12:04 +0100
Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> On Wed, 29 May 2024 15:10:42 +0200
> Julien Stephan <jstephan@...libre.com> wrote:
>
> > Le mer. 29 mai 2024 à 14:05, Jonathan Cameron
> > <Jonathan.Cameron@...wei.com> a écrit :
> > >
> > > On Wed, 29 May 2024 13:55:52 +0200
> > > Julien Stephan <jstephan@...libre.com> 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@...log.com>
> > > > Signed-off-by: Julien Stephan <jstephan@...libre.com>
> > >
> > > 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.
Even if we make further improvements this patch feels like good hardening.
So applied to the togreg branch of iio.git.
Thanks,
Jonathan
> >
> > 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@baylibre.com
> > > > ---
> > > > 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,
> > >
>
Powered by blists - more mailing lists