Re: [RFC PATCH 04/14] iio/light: add new driver for macsmc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] iio: light: unvalidated drv->size in macsmc_light_read_raw() overflows buffer
- [Medium] iio: light: double byte-swap and wrong shift in macsmc_light_read_raw()
- [Medium] iio: light: missing event config callbacks in macsmc_light_info
- [Medium] iio: light: macsmc_light_probe() unconditionally registers RIGHT_LIGHT channel
- [Medium] iio: light: macsmc_light_channels omits indexed flag and uses extend_name
--

commit 03cf99563b174ff5a5c22b832e325621b25e1bec
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

iio/light: add new driver for macsmc

This commit adds an IIO ambient light sensor driver for Apple SMC devices
with left and right illuminance channels and change event support, and
registers the corresponding MFD cell in macsmc-core.

> diff --git a/drivers/iio/light/macsmc-light.c b/drivers/iio/light/macsmc-light.c
> new file mode 100644
> index 0000000000000..9d4532b34ef05
> --- /dev/null
> +++ b/drivers/iio/light/macsmc-light.c
> @@ -0,0 +1,189 @@

[ ... ]

> +static int macsmc_light_read_raw(struct iio_dev *indio_dev,
> +		const struct iio_chan_spec *chan,
> +		int *val, int *val2, long mask)
> +
> +{
> +	struct macsmc_light *drv = iio_priv(indio_dev);
> +	u8 buffer[10];
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	switch (chan->channel) {
> +	case LEFT_LIGHT:
> +		ret = apple_smc_read(drv->smc, SMC_KEY(ALV0), buffer, drv->size);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (drv->size == 10)
> +			*val = be16_to_cpu(get_unaligned_be16(buffer + 6)) >> 10;

[Severity: Medium]
Does this double-swap the endianness and shift away the sensor reading when
drv->size == 10?

In include/linux/unaligned.h, get_unaligned_be16() already converts the
big-endian value to CPU byte order:

static inline u16 get_unaligned_be16(const void *p)
{
	return be16_to_cpu(__get_unaligned_t(__be16, p));
}

Passing its return value to be16_to_cpu() swaps the bytes a second time on
little-endian systems (and triggers a Sparse endianness warning).

In addition, in drivers/hwmon/applesmc.c, applesmc_light_show() handled the
10-byte ALV0 payload as a 10-bit big-endian value at buffer + 6 and shifted
it right by 2 only to fit into an 8-bit variable:

	/* newer macbooks report a single 10-bit bigendian value */
	if (data_length == 10) {
		left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
		goto out;
	}

Shifting a 10-bit value right by 10 bits discards the entire reading. Should
this just be get_unaligned_be16(buffer + 6) (or shifted right by 2 if 8-bit
scaling is intended)?

> +		else
> +			*val = buffer[2];
> +
> +		break;
> +	case RIGHT_LIGHT:
> +		if (drv->size == 10) {
> +			*val = 0;
> +			break;
> +		}
> +		ret = apple_smc_read(drv->smc, SMC_KEY(ALV1), buffer, drv->size);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = buffer[2];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}

[ ... ]

> +static const struct iio_event_spec macsmc_light_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_CHANGE,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec macsmc_light_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.channel = LEFT_LIGHT,
> +		.extend_name = "left",
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.event_spec = macsmc_light_events,
> +		.num_event_specs = ARRAY_SIZE(macsmc_light_events),
> +	},
> +	{
> +		.type = IIO_LIGHT,
> +		.channel = RIGHT_LIGHT,
> +		.extend_name = "right",
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.event_spec = macsmc_light_events,
> +		.num_event_specs = ARRAY_SIZE(macsmc_light_events),
> +	},
> +};

[Severity: Medium]
Should macsmc_light_channels set .indexed = 1 (and use .read_label instead of
the deprecated .extend_name field)?

In drivers/iio/industrialio-core.c, __iio_chan_prefix_emit() only includes
chan->channel in the sysfs attribute name when chan->indexed is set:

		if (chan->indexed)
			seq_buf_printf(&s, "%d", chan->channel);

Without .indexed = 1, the channel numbers are omitted from the sysfs
attribute names, whereas macsmc_light_event() pushes events using
IIO_UNMOD_EVENT_CODE(IIO_LIGHT, LEFT_LIGHT, ...) and
IIO_UNMOD_EVENT_CODE(IIO_LIGHT, RIGHT_LIGHT, ...) with channel indices 0
and 1.

> +
> +static const struct iio_info macsmc_light_info = {
> +	.read_raw = macsmc_light_read_raw,
> +};

[Severity: Medium]
Does macsmc_light_info need .read_event_config and .write_event_config
callbacks?

Because macsmc_light_events sets .mask_separate = BIT(IIO_EV_INFO_ENABLE),
the IIO core creates event enable sysfs attributes wired to
iio_ev_state_show() and iio_ev_state_store() in
drivers/iio/industrialio-event.c:

	if (!indio_dev->info->write_event_config)
		return -EINVAL;

and:

	if (!indio_dev->info->read_event_config)
		return -EINVAL;

With both callbacks left NULL in macsmc_light_info, reading or writing the
event enable attributes in sysfs will always fail with -EINVAL, and
macsmc_light_event() pushes events unconditionally regardless of whether
events are enabled.

> +
> +static int macsmc_light_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct macsmc_light *drv;
> +	struct apple_smc_key_info info;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*drv));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	drv = iio_priv(indio_dev);
> +	drv->smc = dev_get_drvdata(pdev->dev.parent);
> +
> +	ret = apple_smc_get_key_info(drv->smc, SMC_KEY(ALV0), &info);
> +	if (ret < 0)
> +		return -ENODEV;
> +
> +	drv->size = info.size;

[Severity: High]
Can an unexpected info.size from the SMC overflow buffer[] or leak
uninitialized stack memory in macsmc_light_read_raw()?

In macsmc_light_read_raw(), buffer is a 10-byte uninitialized stack array:

	u8 buffer[10];
	...
	ret = apple_smc_read(drv->smc, SMC_KEY(ALV0), buffer, drv->size);

In drivers/hwmon/applesmc.c, applesmc_light_show() checked that the key
length did not exceed 10 before reading into buffer. Here drv->size is taken
directly from info.size without bounds checking. If info.size > 10,
apple_smc_read() will write past the end of buffer[] on the stack; if
info.size < 3, reading buffer[2] will return uninitialized stack bytes to
userspace (and ALV1 is also read using drv->size without checking ALV1's
own key size).

> +	drv->indio_dev = indio_dev;
> +	drv->nb.notifier_call = macsmc_light_event;
> +	ret = blocking_notifier_chain_register(drv->smc->event_handlers, &drv->nb);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, macsmc_light_unregister_notifier, drv);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "macsmc-light";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &macsmc_light_info;
> +	indio_dev->channels = macsmc_light_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(macsmc_light_channels);

[Severity: Medium]
Should RIGHT_LIGHT only be registered when a right sensor is actually
present?

macsmc_light_probe() unconditionally registers both LEFT_LIGHT and
RIGHT_LIGHT channels. On systems where drv->size == 10, there is only a
single sensor in ALV0, so the right channel always returns a hardcoded 0 in
macsmc_light_read_raw() and never receives events in macsmc_light_event().
On systems where drv->size != 10 but SMC_KEY(ALV1) is absent, reading
RIGHT_LIGHT in macsmc_light_read_raw() will fail when querying ALV1, while
macsmc_light_event() will still push change events for RIGHT_LIGHT.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=4