| Message ID | 20241125-iio_memset_scan_holes-v1-2-0cb6e98d895c@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: fix information leaks in triggered buffers | expand |
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote: > The 'scan' local struct is used to push data to user space from a > triggered buffer, but it has a hole between the sample (unsigned int) > and the timestamp. This hole is never initialized. > > Initialize the struct to zero before using it to avoid pushing > uninitialized information to userspace. > > Cc: stable@vger.kernel.org > Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > drivers/iio/adc/ti-ads1119.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c > index e9d9d4d46d38..2615a275acb3 100644 > --- a/drivers/iio/adc/ti-ads1119.c > +++ b/drivers/iio/adc/ti-ads1119.c > @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) > unsigned int index; > int ret; > > + memset(&scan, 0, sizeof(scan)); Did you consider adding a reserved field after sample and just initializing that one to zero? It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it. In any case, the change is fine. Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> Thanks, Francesco
On 26/11/2024 09:59, Francesco Dolcini wrote: > On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote: >> The 'scan' local struct is used to push data to user space from a >> triggered buffer, but it has a hole between the sample (unsigned int) >> and the timestamp. This hole is never initialized. >> >> Initialize the struct to zero before using it to avoid pushing >> uninitialized information to userspace. >> >> Cc: stable@vger.kernel.org >> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> --- >> drivers/iio/adc/ti-ads1119.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c >> index e9d9d4d46d38..2615a275acb3 100644 >> --- a/drivers/iio/adc/ti-ads1119.c >> +++ b/drivers/iio/adc/ti-ads1119.c >> @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) >> unsigned int index; >> int ret; >> >> + memset(&scan, 0, sizeof(scan)); > > Did you consider adding a reserved field after sample and just > initializing that one to zero? > > It seems a trivial optimization not adding much value, but I thought about > it, so I'd like to be sure you considered it. > > In any case, the change is fine. > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > Thanks, > Francesco > Hi Francesco, thanks for your review. In this particular case where unsigned int is used for the sample, the padding would _in theory_ depend on the architecture. The size of the unsigned int is usually 4 bytes, but the standard only specifies that it must be able to contain values in the [0, 65535] range i.e. 2 bytes. That is indeed theory, and I don't know if there is a real case where a new version of Linux is able to run on an architecture that uses 2 bytes for an int. I guess there is not, but better safe than sorry. We could be more specific with u32 for the sample and then add the reserved field, but I would still prefer a memset() for this small struct. Adding and initializing a reserved field looks a bit artificial to me, especially for such marginal gains. Moreover, the common practice (at least in IIO)is a plain memset() to initialize struct holes, and such common patterns are easier to maintain :) Best regards, Javier Carrasco
On Tue, 26 Nov 2024 10:46:37 +0100 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > On 26/11/2024 09:59, Francesco Dolcini wrote: > > On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote: > >> The 'scan' local struct is used to push data to user space from a > >> triggered buffer, but it has a hole between the sample (unsigned int) > >> and the timestamp. This hole is never initialized. > >> > >> Initialize the struct to zero before using it to avoid pushing > >> uninitialized information to userspace. > >> > >> Cc: stable@vger.kernel.org > >> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") > >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > >> --- > >> drivers/iio/adc/ti-ads1119.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c > >> index e9d9d4d46d38..2615a275acb3 100644 > >> --- a/drivers/iio/adc/ti-ads1119.c > >> +++ b/drivers/iio/adc/ti-ads1119.c > >> @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) > >> unsigned int index; > >> int ret; > >> > >> + memset(&scan, 0, sizeof(scan)); > > > > Did you consider adding a reserved field after sample and just > > initializing that one to zero? > > > > It seems a trivial optimization not adding much value, but I thought about > > it, so I'd like to be sure you considered it. > > > > In any case, the change is fine. > > > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > Thanks, > > Francesco > > > > Hi Francesco, thanks for your review. > > In this particular case where unsigned int is used for the sample, the > padding would _in theory_ depend on the architecture. The size of the > unsigned int is usually 4 bytes, but the standard only specifies that it > must be able to contain values in the [0, 65535] range i.e. 2 bytes. > That is indeed theory, and I don't know if there is a real case where a > new version of Linux is able to run on an architecture that uses 2 bytes > for an int. I guess there is not, but better safe than sorry. Using an unsigned int here is a bug as well as we should present consistent formatted data whatever the architecture. > > We could be more specific with u32 for the sample and then add the > reserved field, but I would still prefer a memset() for this small > struct. Adding and initializing a reserved field looks a bit artificial > to me, especially for such marginal gains. Issue with reserved fields is we would have to be very very careful to spot them all. A memset avoids that care being needed. Jonathan > > Moreover, the common practice (at least in IIO)is a plain memset() to > initialize struct holes, and such common patterns are easier to maintain :) > > Best regards, > Javier Carrasco
On Tue Nov 26, 2024 at 7:52 PM CET, Jonathan Cameron wrote: > On Tue, 26 Nov 2024 10:46:37 +0100 > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > > On 26/11/2024 09:59, Francesco Dolcini wrote: > > > On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote: > > >> The 'scan' local struct is used to push data to user space from a > > >> triggered buffer, but it has a hole between the sample (unsigned int) > > >> and the timestamp. This hole is never initialized. > > >> > > >> Initialize the struct to zero before using it to avoid pushing > > >> uninitialized information to userspace. > > >> > > >> Cc: stable@vger.kernel.org > > >> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") > > >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > >> --- > > >> drivers/iio/adc/ti-ads1119.c | 2 ++ > > >> 1 file changed, 2 insertions(+) > > >> > > >> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c > > >> index e9d9d4d46d38..2615a275acb3 100644 > > >> --- a/drivers/iio/adc/ti-ads1119.c > > >> +++ b/drivers/iio/adc/ti-ads1119.c > > >> @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) > > >> unsigned int index; > > >> int ret; > > >> > > >> + memset(&scan, 0, sizeof(scan)); > > > > > > Did you consider adding a reserved field after sample and just > > > initializing that one to zero? > > > > > > It seems a trivial optimization not adding much value, but I thought about > > > it, so I'd like to be sure you considered it. > > > > > > In any case, the change is fine. > > > > > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > > > Thanks, > > > Francesco > > > > > > > Hi Francesco, thanks for your review. > > > > In this particular case where unsigned int is used for the sample, the > > padding would _in theory_ depend on the architecture. The size of the > > unsigned int is usually 4 bytes, but the standard only specifies that it > > must be able to contain values in the [0, 65535] range i.e. 2 bytes. > > That is indeed theory, and I don't know if there is a real case where a > > new version of Linux is able to run on an architecture that uses 2 bytes > > for an int. I guess there is not, but better safe than sorry. > Using an unsigned int here is a bug as well as we should present consistent > formatted data whatever the architecture. Would you prefer that in the same patch as they are related issues? I could switch to u32 in v2 along with anything else that might arise in the reviews of the rest of the series. If you prefer a separate patch, that's fine too. > > > > We could be more specific with u32 for the sample and then add the > > reserved field, but I would still prefer a memset() for this small > > struct. Adding and initializing a reserved field looks a bit artificial > > to me, especially for such marginal gains. > Issue with reserved fields is we would have to be very very careful to spot them > all. A memset avoids that care being needed. > > Jonathan > > > > > Moreover, the common practice (at least in IIO)is a plain memset() to > > initialize struct holes, and such common patterns are easier to maintain :) > > > > Best regards, > > Javier Carrasco
On 26/11/2024 23:00, Javier Carrasco wrote: > On Tue Nov 26, 2024 at 7:52 PM CET, Jonathan Cameron wrote: >> On Tue, 26 Nov 2024 10:46:37 +0100 >> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: >> >>> On 26/11/2024 09:59, Francesco Dolcini wrote: >>>> On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote: >>>>> The 'scan' local struct is used to push data to user space from a >>>>> triggered buffer, but it has a hole between the sample (unsigned int) >>>>> and the timestamp. This hole is never initialized. >>>>> >>>>> Initialize the struct to zero before using it to avoid pushing >>>>> uninitialized information to userspace. >>>>> >>>>> Cc: stable@vger.kernel.org >>>>> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") >>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >>>>> --- >>>>> drivers/iio/adc/ti-ads1119.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c >>>>> index e9d9d4d46d38..2615a275acb3 100644 >>>>> --- a/drivers/iio/adc/ti-ads1119.c >>>>> +++ b/drivers/iio/adc/ti-ads1119.c >>>>> @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) >>>>> unsigned int index; >>>>> int ret; >>>>> >>>>> + memset(&scan, 0, sizeof(scan)); >>>> >>>> Did you consider adding a reserved field after sample and just >>>> initializing that one to zero? >>>> >>>> It seems a trivial optimization not adding much value, but I thought about >>>> it, so I'd like to be sure you considered it. >>>> >>>> In any case, the change is fine. >>>> >>>> Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> >>>> >>>> Thanks, >>>> Francesco >>>> >>> >>> Hi Francesco, thanks for your review. >>> >>> In this particular case where unsigned int is used for the sample, the >>> padding would _in theory_ depend on the architecture. The size of the >>> unsigned int is usually 4 bytes, but the standard only specifies that it >>> must be able to contain values in the [0, 65535] range i.e. 2 bytes. >>> That is indeed theory, and I don't know if there is a real case where a >>> new version of Linux is able to run on an architecture that uses 2 bytes >>> for an int. I guess there is not, but better safe than sorry. >> Using an unsigned int here is a bug as well as we should present consistent >> formatted data whatever the architecture. > > Would you prefer that in the same patch as they are related issues? I > could switch to u32 in v2 along with anything else that might arise in > the reviews of the rest of the series. > If you prefer a separate patch, that's fine too. > Although now that I am looking into it, and according to the datasheet and defined scan_type, the right size should be s16. >>> >>> We could be more specific with u32 for the sample and then add the >>> reserved field, but I would still prefer a memset() for this small >>> struct. Adding and initializing a reserved field looks a bit artificial >>> to me, especially for such marginal gains. >> Issue with reserved fields is we would have to be very very careful to spot them >> all. A memset avoids that care being needed. >> >> Jonathan >> >>> >>> Moreover, the common practice (at least in IIO)is a plain memset() to >>> initialize struct holes, and such common patterns are easier to maintain :) >>> >>> Best regards, >>> Javier Carrasco >
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret; + memset(&scan, 0, sizeof(scan)); + if (!iio_trigger_using_own(indio_dev)) { index = find_first_bit(indio_dev->active_scan_mask, iio_get_masklength(indio_dev));
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized. Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace. Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)