On Sun, 24 Nov 2024 15:47:23 +0100
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
> On 24/11/2024 13:43, Jonathan Cameron wrote:
> >>>> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
> >>>> +{
> >>>> + struct iio_poll_func *pf = p;
> >>>> + struct iio_dev *iio = pf->indio_dev;
> >>>> + struct veml6030_data *data = iio_priv(iio);
> >>>> + unsigned int reg;
> >>>> + int ch, ret, i = 0;
> >>>> + struct {
> >>>> + u16 chans[2];
> >>> There is a hole here...
> >>>> + aligned_s64 timestamp;
> >>>> + } scan;
> >>>> +
> >>>> + iio_for_each_active_channel(iio, ch) {
> >>>> + ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
> >>>> + ®);
> >>>> + if (ret)
> >>>> + goto done;
> >>>> +
> >>>> + scan.chans[i++] = reg;
> >>> This fills in at least 1 channel, but maybe not the second.
> >>>> + }
> >>>> +
> >>> So this leaks random stack data I think.
> >>>
> >>> Upshot, when holes are involved or not all the channels are set, need
> >>> memset(&scan, 0, sizeof(scan));
> >>> for the structure on the stack which will zero the holes as well as
> >>> both channels.
> >>>
> >>> Ancient article on this: https://lwn.net/Articles/417989/
> >>>
> >>> We get away with it when they are in the iio_priv space because they are
> >>> kzalloc + if we do leak data due to changes in configured channels it's
> >>> just old sensor data which is (I think) never a security problem!
> >>>
> >>>> + iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
> >>>> +
> >>>> +done:
> >>>> + iio_trigger_notify_done(iio->trig);
> >>>> +
> >>>> + return IRQ_HANDLED;
> >>>> +}
> >>>> +
> >>>> static int veml6030_set_info(struct iio_dev *indio_dev)
> >>>> {
> >>>> struct veml6030_data *data = iio_priv(indio_dev);
> >>>> @@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
> >>>> if (ret < 0)
> >>>> return ret;
> >>>>
> >>>> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> >>>> + veml6030_trigger_handler, NULL);
> >>>> + if (ret)
> >>>> + return dev_err_probe(&client->dev, ret,
> >>>> + "Failed to register triggered buffer");
> >>>> +
> >>>> return devm_iio_device_register(&client->dev, indio_dev);
> >>>> }
> >>>>
> >>>>
> >>>> ---
> >>>> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
> >>>> change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce
> >>>>
> >>>> Best regards,
> >>>
> >>
> >>
> >> Hi Jonathan,
> >>
> >> thanks a lot for your explanation and the link, it makes perfect sense.
> >> By the way, when I moved this struct from the iio_priv to the function,
> >> I took a look at some existing code, and a couple of them might have the
> >> same issue:
> >>
> >> - temperature/tmp006.c: it also has a hole between the two 16-bit
> >> channels and the timestamp (aligned(8)), but it is not set to zero.
> >>
> >> - adc/ti-ads1119.c: the scan consists of an unsigned int and the
> >> timestamp (aligned(8)). I believe there is a hole there as well.
> >>
> >> I did not go over all drivers (most of them store the scan struct in the
> >> iio_priv space anyway), but at least those two look suspicious.
> >>
> >> Should I fix (e.g. memset) those two I mentioned?
> >
> > Please do. Thanks!
> >
> > Jonathan
> >
>
> Ok, I will take a closer look to check if there are more drivers leaking
> uninitialized data. By the way, would you tag the fixes for stable? This
> becoming an attack vector might be a bit theoretical, and I don't know
> the consensus about the danger of passing uninitialized data to userspace.
Yes. Stable would be appropriate for these. Thanks for looking into it.
Your patch had me adding checking this to my todo list but I'm very
happy to have you do it instead! :)
Jonathan
>
> Thanks again,
> Javier Carrasco
