| Message ID | 1520534733-1743-1-git-send-email-himanshujha199640@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 8, 2018 at 10:45 AM, Himanshu Jha <himanshujha199640@gmail.com> wrote: > In preparation to enabling -Wvla, remove VLA usage and replace it > with fixed a fixed length array and therefore, prevent potential > stack overflow attacks. > > Fixed as a part of the discussion to remove all VLAs from the kernel: > https://lkml.org/lkml/2018/3/7/621 > > Cc: keescook@chromium.org > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> > --- > drivers/iio/potentiometer/ds1803.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c > index 9b0ff4a..6bf12c9 100644 > --- a/drivers/iio/potentiometer/ds1803.c > +++ b/drivers/iio/potentiometer/ds1803.c > @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, > struct ds1803_data *data = iio_priv(indio_dev); > int pot = chan->channel; > int ret; > - u8 result[indio_dev->num_channels]; > + u8 result[ARRAY_SIZE(ds1803_channels)]; It seems like num_channels is always ARRAY_SIZE(ds1803_channels). Could the entire field be dropped? -Kees > > switch (mask) { > case IIO_CHAN_INFO_RAW: > -- > 2.7.4 >
On Thu, Mar 08, 2018 at 11:39:15AM -0800, Kees Cook wrote: > On Thu, Mar 8, 2018 at 10:45 AM, Himanshu Jha > <himanshujha199640@gmail.com> wrote: > > In preparation to enabling -Wvla, remove VLA usage and replace it > > with fixed a fixed length array and therefore, prevent potential > > stack overflow attacks. > > > > Fixed as a part of the discussion to remove all VLAs from the kernel: > > https://lkml.org/lkml/2018/3/7/621 > > > > Cc: keescook@chromium.org > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> > > --- > > drivers/iio/potentiometer/ds1803.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c > > index 9b0ff4a..6bf12c9 100644 > > --- a/drivers/iio/potentiometer/ds1803.c > > +++ b/drivers/iio/potentiometer/ds1803.c > > @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, > > struct ds1803_data *data = iio_priv(indio_dev); > > int pot = chan->channel; > > int ret; > > - u8 result[indio_dev->num_channels]; > > + u8 result[ARRAY_SIZE(ds1803_channels)]; > > It seems like num_channels is always ARRAY_SIZE(ds1803_channels). > Could the entire field be dropped? If you're asking to remove num_channels then certainly it is not possible since it is a member of industrial I/O device struct and it is not just a member of regular struct local to this file. We certainly know that there are only two channels BTW and it can be tranformed to simply: u8 result[2]; But then I might have to add an additional comment explaining the magic number 2.
On Fri, 9 Mar 2018 16:35:10 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Thu, Mar 08, 2018 at 11:39:15AM -0800, Kees Cook wrote: > > On Thu, Mar 8, 2018 at 10:45 AM, Himanshu Jha > > <himanshujha199640@gmail.com> wrote: > > > In preparation to enabling -Wvla, remove VLA usage and replace it > > > with fixed a fixed length array and therefore, prevent potential > > > stack overflow attacks. > > > > > > Fixed as a part of the discussion to remove all VLAs from the kernel: > > > https://lkml.org/lkml/2018/3/7/621 > > > > > > Cc: keescook@chromium.org > > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> > > > --- > > > drivers/iio/potentiometer/ds1803.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c > > > index 9b0ff4a..6bf12c9 100644 > > > --- a/drivers/iio/potentiometer/ds1803.c > > > +++ b/drivers/iio/potentiometer/ds1803.c > > > @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, > > > struct ds1803_data *data = iio_priv(indio_dev); > > > int pot = chan->channel; > > > int ret; > > > - u8 result[indio_dev->num_channels]; > > > + u8 result[ARRAY_SIZE(ds1803_channels)]; > > > > It seems like num_channels is always ARRAY_SIZE(ds1803_channels). > > Could the entire field be dropped? > > If you're asking to remove num_channels then certainly it is not > possible > since it is a member of industrial I/O device struct and it is not just > a member of regular struct local to this file. > > We certainly know that there are only two channels BTW and it can be > tranformed to simply: > > u8 result[2]; > > But then I might have to add an additional comment explaining the magic > number 2. I'm happy with the exact version you proposed. num_channels isn't there for the driver use (as it can obviously know this) but for the core which uses this to know how big the channel array is when creating the sysfs interfaces etc. Applied to the togreg branch of iio.git and pushed out as testing. Thanks, Jonathan > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c index 9b0ff4a..6bf12c9 100644 --- a/drivers/iio/potentiometer/ds1803.c +++ b/drivers/iio/potentiometer/ds1803.c @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, struct ds1803_data *data = iio_priv(indio_dev); int pot = chan->channel; int ret; - u8 result[indio_dev->num_channels]; + u8 result[ARRAY_SIZE(ds1803_channels)]; switch (mask) { case IIO_CHAN_INFO_RAW:
In preparation to enabling -Wvla, remove VLA usage and replace it with fixed a fixed length array and therefore, prevent potential stack overflow attacks. Fixed as a part of the discussion to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621 Cc: keescook@chromium.org Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> --- drivers/iio/potentiometer/ds1803.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)