| 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: <aQOVcCinTd-ZJJX3@lore-desk>
Date: Thu, 30 Oct 2025 17:42:24 +0100
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Francesco Lavra <flavra@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
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 1/9] iio: imu: st_lsm6dsx: dynamically initialize
iio_chan_spec data
> Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> for the iio_chan_spec struct arrays makes all sensors advertise
> channel event capabilities regardless of whether they actually
> support event generation. And if userspace tries to configure
> accelerometer wakeup events on a sensor device that does not
> support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> a NULL pointer when trying to write to the wakeup register.
> Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> initialization of struct iio_chan_spec arrays, where the
> st_lsm6dsx_event structure is only used for sensors that support
> wakeup events; besides fixing the above bug, this serves as a
> preliminary step for adding support for more event types.
I agree we are missing the Fixes tag here.
>
> Signed-off-by: Francesco Lavra <flavra@...libre.com>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 26 +--
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 ++++++++-----------
> 2 files changed, 71 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index a4f558899767..db863bd1898d 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -80,27 +80,6 @@ enum st_lsm6dsx_hw_id {
> * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
> #define ST_LSM6DSX_SHIFT_VAL(val, mask) (((val) << __ffs(mask)) & (mask))
>
> -#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx) \
> -{ \
> - .type = chan_type, \
> - .address = addr, \
> - .modified = 1, \
> - .channel2 = mod, \
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> - .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> - .scan_index = scan_idx, \
> - .scan_type = { \
> - .sign = 's', \
> - .realbits = 16, \
> - .storagebits = 16, \
> - .endianness = IIO_LE, \
> - }, \
> - .event_spec = &st_lsm6dsx_event, \
> - .ext_info = st_lsm6dsx_ext_info, \
> - .num_event_specs = 1, \
> -}
> -
> #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx) \
> { \
> .type = chan_type, \
> @@ -328,10 +307,7 @@ struct st_lsm6dsx_settings {
> const char *name;
> u8 wai;
> } id[ST_LSM6DSX_MAX_ID];
> - struct {
> - const struct iio_chan_spec *chan;
> - int len;
> - } channels[2];
> + u8 chan_addr_base[2];
nit: chan_addr[2]
> struct {
> struct st_lsm6dsx_reg irq1;
> struct st_lsm6dsx_reg irq2;
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 216160549b5a..17b46e15cce5 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -96,26 +96,7 @@
>
> #define ST_LSM6DSX_TS_SENSITIVITY 25000UL /* 25us */
>
> -static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> - ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
> - ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
> - ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2c, IIO_MOD_Z, 2),
> - IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> -
> -static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x22, IIO_MOD_X, 0),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x24, IIO_MOD_Y, 1),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x26, IIO_MOD_Z, 2),
> - IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> -
> -static const struct iio_chan_spec st_lsm6ds0_gyro_channels[] = {
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x18, IIO_MOD_X, 0),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1a, IIO_MOD_Y, 1),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1c, IIO_MOD_Z, 2),
> - IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> +#define ST_LSM6DSX_CHAN_COUNT 4
>
> static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> {
> @@ -142,15 +123,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x68,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6ds0_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6ds0_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x18,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -246,15 +221,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x69,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -412,15 +381,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x69,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -590,15 +553,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x6a,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -847,15 +804,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x6d,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -1060,15 +1011,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x6b,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -1237,15 +1182,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x70,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -1443,15 +1382,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x22,
> }
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -2366,21 +2299,64 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> return st_lsm6dsx_init_hw_timer(hw);
> }
>
> +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6dsx_hw *hw,
> + enum st_lsm6dsx_sensor_id id, int index)
please try to respect the 79 column limit (I still like it :))
> +{
> + struct iio_chan_spec *chan = &channels[index];
> +
> + chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;
I think you should return an error here if id is not ST_LSM6DSX_ID_ACC or
ST_LSM6DSX_ID_GYRO.
> + chan->address = hw->settings->chan_addr_base[id] + index * ST_LSM6DSX_CHAN_SIZE;
> + chan->modified = 1;
> + chan->channel2 = IIO_MOD_X + index;
> + chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> + chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> + chan->scan_index = index;
> + chan->scan_type.sign = 's';
> + chan->scan_type.realbits = 16;
> + chan->scan_type.storagebits = 16;
> + chan->scan_type.endianness = IIO_LE;
what about reducing the scope of ST_LSM6DSX_CHANNEL_ACC/ST_LSM6DSX_CHANNEL here
to improve the iio_chan_spec struct initialization since most of the fields are
always the same between different sensors.
> + chan->ext_info = st_lsm6dsx_ext_info;
> + if (id == ST_LSM6DSX_ID_ACC) {
> + if (hw->settings->event_settings.wakeup_reg.addr) {
if (id == ST_LSM6DSX_ID_ACC &&
hw->settings->event_settings.wakeup_reg.addr) {
...
}
> + chan->event_spec = &st_lsm6dsx_event;
> + chan->num_event_specs = 1;
> + }
> + }
> + return 0;
> +}
> +
> static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> enum st_lsm6dsx_sensor_id id,
> const char *name)
> {
> struct st_lsm6dsx_sensor *sensor;
> struct iio_dev *iio_dev;
> + struct iio_chan_spec *channels;
nit: chan to be consistent
> + int i;
>
> iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
> if (!iio_dev)
> return NULL;
>
> + channels = devm_kzalloc(hw->dev, sizeof(*channels) * ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
79 column limit here. I guess you can use even devm_kcalloc() here.
> + if (!channels)
> + return NULL;
> +
> + for (i = 0; i < 3; i++) {
> + if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> + return NULL;
> + }
new line here.
> + channels[3].type = IIO_TIMESTAMP;
> + channels[3].channel = -1;
> + channels[3].scan_index = 3;
> + channels[3].scan_type.sign = 's';
> + channels[3].scan_type.realbits = 64;
> + channels[3].scan_type.storagebits = 64;
> iio_dev->modes = INDIO_DIRECT_MODE;
> iio_dev->available_scan_masks = st_lsm6dsx_available_scan_masks;
> - iio_dev->channels = hw->settings->channels[id].chan;
> - iio_dev->num_channels = hw->settings->channels[id].len;
> + iio_dev->channels = channels;
> + iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;
>
> sensor = iio_priv(iio_dev);
> sensor->id = id;
> --
> 2.39.5
>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists