Re: [PATCH] iio: st_sensors: add __counted_by_ptr to struct st_sensor_settings

[Bill Wendling]

On Wed, Sep 23, 2026 at 12:34 PM Bill Wendling <morbo@xxxxxxxxxx> wrote:
> On Wed, Sep 23, 2026 at 12:52 AM Kees Cook <kees@xxxxxxxxxx> wrote:
> > On Tue, Sep 22, 2026 at 11:21:11AM +0000, Bill Wendling wrote:
> > > Annotate the "ch" pointer member of "struct st_sensor_settings" with the
> > > "__counted_by_ptr" attribute. The elements of "ch" are counted by the
> > > "num_ch" member in the same struct.
> > >
> > > All instances of "struct st_sensor_settings" are defined as "static
> > > const" arrays across the ST sensor core drivers. For pressure sensors,
> > > "num_ch" is explicitly initialized with the size of the respective
> > > channel array using "ARRAY_SIZE(...)" during static definition.
> >
> > I wanted to understand this so, in drivers/iio/accel/st_accel_core.c:
> >
> > static const struct st_sensor_settings st_accel_sensors_settings[] = {
> > ...
> >                 .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> > ...
> > }
> >
> > static const struct iio_chan_spec st_accel_12bit_channels[] = {
> >         ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
> >                         BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> >                         ST_SENSORS_SCAN_X, 1, IIO_MOD_X, 's', IIO_LE, 12, 16,
> >                         ST_ACCEL_DEFAULT_OUT_X_L_ADDR,
> >                         st_accel_mount_matrix_ext_info),
> >         ...
> >         IIO_CHAN_SOFT_TIMESTAMP(3)
> > };
> >
> > #define IIO_CHAN_SOFT_TIMESTAMP(_si) (struct iio_chan_spec) {           \
> >         .type = IIO_TIMESTAMP,                                          \
> >         .channel = -1,                                                  \
> >         .scan_index = _si,                                              \
> >         .scan_type = {                                                  \
> >                 .sign = 's',                                            \
> >                 .realbits = 64,                                         \
> >                 .storagebits = 64,                                      \
> >         },                                                              \
> > }
> >
> > So .ch is assigned an array of items which looks to end with a termination
> > sentinel. And .num_ch is unassigned:
> >
> > $ git grep '\.ch = ' drivers/iio/accel/st_accel_core.c | wc -l
> > 16
> > $ git grep '\.num_ch = ' drivers/iio/accel/st_accel_core.c | wc -l
> > 0
> >
> > > For accelerometer, gyroscope, and magnetometer sensors, "num_ch" is not
> > > explicitly initialized (and thus defaults to 0). This is because those
> > > drivers hardcode the channel count to "ST_SENSORS_NUMBER_ALL_CHANNELS"
> > > rather than using "num_ch" from the settings struct.
> >
> > Yeah, checks out:
> >
> > $ git grep '\.ch = ' | cut -d: -f1 | sort | uniq -c
> >      16 accel/st_accel_core.c
> >       4 gyro/st_gyro_core.c
> >       5 magnetometer/st_magn_core.c
> >       6 pressure/st_pressure_core.c
> > $ git grep '\.num_ch = ' | cut -d: -f1 | sort | uniq -c
> >       6 pressure/st_pressure_core.c
> >
> > Interestingly, the pressure/st_pressure_core.c uses both assigned .num_ch
> > _and_ a sentinel.
> >
> > > Since these structures are static const, both "ch" and "num_ch" are
> > > fully initialized at compile time and available immediately at boot
> > > time.  The only accesses to the "ch" field of "st_sensor_settings" occur
> > > when assigning it to "indio_dev->channels" during device probing.
> >
> > Heh, so the bounds check gets laundered. :) Even pressure:
> >
> > pressure/st_pressure_core.c:    press_data->num_data_channels = press_data->sensor_settings->num_ch - 1;
> > pressure/st_pressure_core.c:    indio_dev->num_channels = press_data->sensor_settings->num_ch;
> >
> > > Because "sensor_settings->ch" is never dereferenced or accessed as an
> > > array, adding the "__counted_by_ptr" annotation does not cause any
> > > runtime panics or false-positive bounds checks under KASAN or UBSAN.
> >
> > I would argue that either __counted_by_ptr has no use here ("accidentally
> > safe because no one dereferences .ch" isn't a great justification)
> > or that the accelerometer, gyroscope, and magnetometer sensors should
> > have their .num_ch assigned correctly so that the "contract" is correct,
> > even if nothing does the deref.
> >
> This is all very weird. Below is the full output from CodeMender. It
> might give some insight into this, but perhaps we should just NACK
> this patch?
>
After further review, I'm going to NACK this patch as having no
effect. I think this makes me a real kernel developer now? :-)

NACK: Bill Wendling <morbo@xxxxxxxxxx>

-bw