[PATCH] iio:core: Fix bug in length of event info_mask and catch unhandled bits set in masks.

[PATCH] iio:core: Fix bug in length of event info_mask and catch unhandled bits set in masks.

[Jonathan Cameron]

The unhandled bits case was highlighted by smatch:
  CHECK   drivers/iio/industrialio-core.c
drivers/iio/industrialio-core.c:719 iio_device_add_info_mask_type() error: buffer overflow 'iio_chan_info_postfix' 17 <= 31
  CC [M]  drivers/iio/industrialio-core.o
  CHECK   drivers/iio/industrialio-event.c
drivers/iio/industrialio-event.c:327 iio_device_add_event() error: buffer overflow 'iio_ev_info_text' 3 <= 3

The incorrect limit for the for_each_set_bit loop was noticed whilst fixing
this other case.  Note that as we only have 3 possible entries a the moment
and the value was set to 4, the bug would not have any effect currently.
It will bite fairly soon though, so best fix it now.

Signed-off-by: Jonathan Cameron <jic23@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/iio/industrialio-core.c  | 2 ++
 drivers/iio/industrialio-event.c | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index ede16aec..e4961b1 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -716,6 +716,8 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
 	int i, ret, attrcount = 0;
 
 	for_each_set_bit(i, infomask, sizeof(infomask)*8) {
+		if (i >= ARRAY_SIZE(iio_chan_info_postfix))
+			return -EINVAL;
 		ret = __iio_add_chan_devattr(iio_chan_info_postfix[i],
 					     chan,
 					     &iio_read_channel_info,
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index ea6e06b..dddfb0f 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -321,7 +321,9 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
 	char *postfix;
 	int ret;
 
-	for_each_set_bit(i, mask, sizeof(*mask)) {
+	for_each_set_bit(i, mask, sizeof(*mask)*8) {
+		if (i >= ARRAY_SIZE(iio_ev_info_text))
+			return -EINVAL;
 		postfix = kasprintf(GFP_KERNEL, "%s_%s_%s",
 				iio_ev_type_text[type], iio_ev_dir_text[dir],
 				iio_ev_info_text[i]);
-- 
1.9.0

Re: [PATCH] iio:core: Fix bug in length of event info_mask and catch unhandled bits set in masks.

[Jonathan Cameron]

On 01/03/14 22:24, Jonathan Cameron wrote:
> The unhandled bits case was highlighted by smatch:
>    CHECK   drivers/iio/industrialio-core.c
> drivers/iio/industrialio-core.c:719 iio_device_add_info_mask_type() error: buffer overflow 'iio_chan_info_postfix' 17 <= 31
>    CC [M]  drivers/iio/industrialio-core.o
>    CHECK   drivers/iio/industrialio-event.c
> drivers/iio/industrialio-event.c:327 iio_device_add_event() error: buffer overflow 'iio_ev_info_text' 3 <= 3
>
> The incorrect limit for the for_each_set_bit loop was noticed whilst fixing
> this other case.  Note that as we only have 3 possible entries a the moment
> and the value was set to 4, the bug would not have any effect currently.
> It will bite fairly soon though, so best fix it now.
>
> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
Executive descision time :) Applied to the togreg branch of iio.git
as clearly no one cares.
> ---
>   drivers/iio/industrialio-core.c  | 2 ++
>   drivers/iio/industrialio-event.c | 4 +++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index ede16aec..e4961b1 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -716,6 +716,8 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
>   	int i, ret, attrcount = 0;
>
>   	for_each_set_bit(i, infomask, sizeof(infomask)*8) {
> +		if (i >= ARRAY_SIZE(iio_chan_info_postfix))
> +			return -EINVAL;
>   		ret = __iio_add_chan_devattr(iio_chan_info_postfix[i],
>   					     chan,
>   					     &iio_read_channel_info,
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index ea6e06b..dddfb0f 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -321,7 +321,9 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
>   	char *postfix;
>   	int ret;
>
> -	for_each_set_bit(i, mask, sizeof(*mask)) {
> +	for_each_set_bit(i, mask, sizeof(*mask)*8) {
> +		if (i >= ARRAY_SIZE(iio_ev_info_text))
> +			return -EINVAL;
>   		postfix = kasprintf(GFP_KERNEL, "%s_%s_%s",
>   				iio_ev_type_text[type], iio_ev_dir_text[dir],
>   				iio_ev_info_text[i]);
>

[PATCH] iio:core: Fix bug in length of event info_mask and catch unhandled bits set in masks.

[Young Xiao]

The incorrect limit for the for_each_set_bit loop was noticed whilst fixing
this other case.  Note that as we only have 3 possible entries a the moment
and the value was set to 4, the bug would not have any effect currently.
It will bite fairly soon though, so best fix it now.

See commit ef4b4856593f ("iio:core: Fix bug in length of event info_mask and
catch unhandled bits set in masks.") for details.

Signed-off-by: Young Xiao <92siuyang@gmail.com>
---
 drivers/iio/industrialio-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f5a4581..dd8873a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1107,6 +1107,8 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
 	char *avail_postfix;
 
 	for_each_set_bit(i, infomask, sizeof(*infomask) * 8) {
+		if (i >= ARRAY_SIZE(iio_chan_info_postfix))
+			return -EINVAL;
 		avail_postfix = kasprintf(GFP_KERNEL,
 					  "%s_available",
 					  iio_chan_info_postfix[i]);
-- 
2.7.4

Re: [PATCH] iio:core: Fix bug in length of event info_mask and catch unhandled bits set in masks.

[Ardelean, Alexandru]

On Tue, 2019-06-04 at 20:40 +0800, Young Xiao wrote:
> [External]
> 
> 
> The incorrect limit for the for_each_set_bit loop was noticed whilst fixing
> this other case.  Note that as we only have 3 possible entries a the moment
> and the value was set to 4, the bug would not have any effect currently.
> It will bite fairly soon though, so best fix it now.
> 
> See commit ef4b4856593f ("iio:core: Fix bug in length of event info_mask and
> catch unhandled bits set in masks.") for details.
> 
> Signed-off-by: Young Xiao <92siuyang@gmail.com>

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Thanks for this patch.
This fix is validated also by the fact that iio_device_add_info_mask_type() has this check on the same iteration.


> ---
>  drivers/iio/industrialio-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index f5a4581..dd8873a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1107,6 +1107,8 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
>         char *avail_postfix;
> 
>         for_each_set_bit(i, infomask, sizeof(*infomask) * 8) {
> +               if (i >= ARRAY_SIZE(iio_chan_info_postfix))
> +                       return -EINVAL;
>                 avail_postfix = kasprintf(GFP_KERNEL,
>                                           "%s_available",
>                                           iio_chan_info_postfix[i]);
> --
> 2.7.4
> 

Re: [PATCH] iio:core: Fix bug in length of event info_mask and catch unhandled bits set in masks.

[Jonathan Cameron]

On Thu, 6 Jun 2019 08:59:10 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Tue, 2019-06-04 at 20:40 +0800, Young Xiao wrote:
> > [External]
> > 
> > 
> > The incorrect limit for the for_each_set_bit loop was noticed whilst fixing
> > this other case.  Note that as we only have 3 possible entries a the moment
> > and the value was set to 4, the bug would not have any effect currently.
> > It will bite fairly soon though, so best fix it now.
> > 
> > See commit ef4b4856593f ("iio:core: Fix bug in length of event info_mask and
> > catch unhandled bits set in masks.") for details.
> > 
> > Signed-off-by: Young Xiao <92siuyang@gmail.com>  
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Thanks for this patch.
> This fix is validated also by the fact that iio_device_add_info_mask_type() has this check on the same iteration.

I don't think it is technically a bug, as the higher bits should never be set.
Still it is a sensible bit of hardening so applied to the togreg branch of iio.git
and pushed out as testing.

Thanks

Jonathan


> 
> 
> > ---
> >  drivers/iio/industrialio-core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index f5a4581..dd8873a 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1107,6 +1107,8 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
> >         char *avail_postfix;
> > 
> >         for_each_set_bit(i, infomask, sizeof(*infomask) * 8) {
> > +               if (i >= ARRAY_SIZE(iio_chan_info_postfix))
> > +                       return -EINVAL;
> >                 avail_postfix = kasprintf(GFP_KERNEL,
> >                                           "%s_available",
> >                                           iio_chan_info_postfix[i]);
> > --
> > 2.7.4
> >