linux-kernel - Re: [PATCH] iio: trigger: Add validation to reject devices requiring top half

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: <CAKbEznvZ3BHJK8TjGg7MR2dDMtWk+gZ5SewF_u_J0=Nw6c082Q@mail.gmail.com>
Date: Wed, 7 May 2025 00:55:27 +0900
From: Gyeyoung Baek <gye976@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: trigger: Add validation to reject devices requiring
 top half

Hello Jonathan, thank you for the review.
I would appreciate it if you could review my additional comments.

On Sun, May 4, 2025 at 11:24 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sun,  4 May 2025 04:00:43 +0900
> Gyeyoung Baek <gye976@...il.com> wrote:
>
> > Some device drivers implement top-half handler,
> > which is not compatible with threaded handler trigger.
> > This patch adds a validation function to reject such devices,
> > allowing only iio_pollfunc_store_time().
>
> This needs more reasoning.  What makes it not work?
> + what do we mean by not compatible?
> I'd expect at least a reference to it using iio_trigger_poll_nested()
> directly.

Of course, even if the IIO device registers a top-half,
`iio_trigger_poll_nested()` ignores the top-half and only calls the
bottom-half, so it works properly.
What I misunderstood here is that I thought there were other IIO
devices implementing a top-half other than
`iio_pollfunc_store_time()`. So I assumed that the TODO was to block
those.
I had confused it with the IIO trigger's top-half handler, apologies
for the confusion.

---

> It's unfortunately hard to tell whether a top half handler is
> actually needed or not.  As a follow up question, what cases do we have
> of top half / interrupt context handlers other than iio_pollfunc_store_time()?

No, it seems that `iio_pollfunc_store_time()` is the only top-half
handler for IIO devices.

---

> Maybe we don't need this code to be this complex any more at all
> (i.e. could it become a flag to say whether the timestamp is useful or not)
> rather than registering the callback.

my new understanding of TODO is as follows:
    - Since `iio_loop_thread()` can only call
`iio_trigger_poll_nested()` and not `iio_trigger_poll()`,
      if the connected IIO device expects a top half such as
`iio_pollfunc_store_time()`,
      then `iio_loop_thread()` needs to directly call
`iio_pollfunc_store_time().`

Would my understanding be correct?

---

> >
> > +/*
> > + * Protect against connection of devices that 'need' the top half
> > + * handler.
> > + */
> > +static int iio_loop_trigger_validate_device(struct iio_trigger *trig,
> > +                                             struct iio_dev *indio_dev)
> > +{
> > +     struct iio_poll_func *pf = indio_dev->pollfunc;
> > +
> > +     /* Only iio timestamp grabbing is allowed. */
> > +     if (pf->h && pf->h != iio_pollfunc_store_time)
>
> Why is iio_pollfunc_store_time useable here?  It's not going to store the
> time if we don't call it...  We could special case it probably but we'd
> need to ensure the call is actually made.

Yes, If my new understanding is correct, `iio_loop_thread()` needs to
call `iio_pollfunc_store_time()` directly,
depending on whether the IIO device's top-half is NULL or
`iio_pollfunc_store_time()`.

--
Regards,
Gyeyoung