On Wed, Sep 23, 2026 at 12:34 PM Bill Wendling <[email protected]> wrote: > On Wed, Sep 23, 2026 at 12:52 AM Kees Cook <[email protected]> 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 <[email protected]> -bw
