[v3] iio: cros_ec: Fix the allocation size for cros_ec_command

Commit Message

yguoaz June 30, 2023, 2:37 p.m. UTC

The struct cros_ec_command contains several integer fields and a
trailing array. An allocation size neglecting the integer fields can
lead to buffer overrun.

Reviewed-by: Tzung-Bi Shih <[e-mail]>
Signed-off-by: Yiyuan Guo <[e-mail]>
---
v2->v3: 
 * Added R-b tag from Tzung-Bi Shih
 * Aligned the code by adding an extra tab before "max"
 * Added a patch changelog
v1->v2: Prefixed the commit title with "iio: cros_ec:"

 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jonathan Cameron July 16, 2023, 1:10 p.m. UTC | #1

On Fri, 30 Jun 2023 22:37:19 +0800
Yiyuan Guo <[e-mail]> wrote:

> The struct cros_ec_command contains several integer fields and a
> trailing array. An allocation size neglecting the integer fields can
> lead to buffer overrun.
> 
> Reviewed-by: Tzung-Bi Shih <[e-mail]>
> Signed-off-by: Yiyuan Guo <[e-mail]>

Hi. I'm sitting on this one for a couple of reasons.
1) No fixes tag (replying to this thread with one is fine)
2) Various people commented on earlier versions, and I'm waiting for them to confirm
they are fine with this version.

If I hear nothing in a few more weeks I'll try and figure out the
fixes tag + whether all the reviewer comments have been addressed.

Jonathan

> ---
> v2->v3: 
>  * Added R-b tag from Tzung-Bi Shih
>  * Aligned the code by adding an extra tab before "max"
>  * Added a patch changelog
> v1->v2: Prefixed the commit title with "iio: cros_ec:"
> 
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 943e9e14d1e9..b72d39fc2434 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -253,7 +253,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	state->ec = ec->ec_dev;
> -	state->msg = devm_kzalloc(&pdev->dev,
> +	state->msg = devm_kzalloc(&pdev->dev, sizeof(*state->msg) +
>  				max((u16)sizeof(struct ec_params_motion_sense),
>  				state->ec->max_response), GFP_KERNEL);
>  	if (!state->msg)

yguoaz July 16, 2023, 1:55 p.m. UTC | #2

Fixes: 974e6f02e27e1b46 ("iio: cros_ec_sensors_core: Add common
functions for the ChromeOS EC S…")

On Sun, Jul 16, 2023 at 9:10 PM Jonathan Cameron <[e-mail]> wrote:
>
> On Fri, 30 Jun 2023 22:37:19 +0800
> Yiyuan Guo <[e-mail]> wrote:
>
> > The struct cros_ec_command contains several integer fields and a
> > trailing array. An allocation size neglecting the integer fields can
> > lead to buffer overrun.
> >
> > Reviewed-by: Tzung-Bi Shih <[e-mail]>
> > Signed-off-by: Yiyuan Guo <[e-mail]>
>
> Hi. I'm sitting on this one for a couple of reasons.
> 1) No fixes tag (replying to this thread with one is fine)
> 2) Various people commented on earlier versions, and I'm waiting for them to confirm
> they are fine with this version.
>
> If I hear nothing in a few more weeks I'll try and figure out the
> fixes tag + whether all the reviewer comments have been addressed.
>
> Jonathan
>
> > ---
> > v2->v3:
> >  * Added R-b tag from Tzung-Bi Shih
> >  * Aligned the code by adding an extra tab before "max"
> >  * Added a patch changelog
> > v1->v2: Prefixed the commit title with "iio: cros_ec:"
> >
> >  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index 943e9e14d1e9..b72d39fc2434 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -253,7 +253,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >       platform_set_drvdata(pdev, indio_dev);
> >
> >       state->ec = ec->ec_dev;
> > -     state->msg = devm_kzalloc(&pdev->dev,
> > +     state->msg = devm_kzalloc(&pdev->dev, sizeof(*state->msg) +
> >                               max((u16)sizeof(struct ec_params_motion_sense),
> >                               state->ec->max_response), GFP_KERNEL);
> >       if (!state->msg)
>

Tzung-Bi Shih July 17, 2023, 3:09 a.m. UTC | #3

On Sun, Jul 16, 2023 at 02:10:28PM +0100, Jonathan Cameron wrote:
[...]
> 2) Various people commented on earlier versions, and I'm waiting for them to confirm
> they are fine with this version.

The version addressed all my comments and LGTM.

Jonathan Cameron July 18, 2023, 9:37 a.m. UTC | #4

On Mon, 17 Jul 2023 11:09:08 +0800
Tzung-Bi Shih <[e-mail]> wrote:

> On Sun, Jul 16, 2023 at 02:10:28PM +0100, Jonathan Cameron wrote:
> [...]
> > 2) Various people commented on earlier versions, and I'm waiting for them to confirm
> > they are fine with this version.  
> 
> The version addressed all my comments and LGTM.

Tag?  I can pick up without, but it's nice to record this
formally.   Reviewed-by seems appropriate here.

Jonathan Cameron July 29, 2023, 11:21 a.m. UTC | #5

On Tue, 18 Jul 2023 10:37:02 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Mon, 17 Jul 2023 11:09:08 +0800
> Tzung-Bi Shih <[e-mail]> wrote:
> 
> > On Sun, Jul 16, 2023 at 02:10:28PM +0100, Jonathan Cameron wrote:
> > [...]  
> > > 2) Various people commented on earlier versions, and I'm waiting for them to confirm
> > > they are fine with this version.    
> > 
> > The version addressed all my comments and LGTM.  
> 
> Tag?  I can pick up without, but it's nice to record this
> formally.   Reviewed-by seems appropriate here.

Applied to the fixes-togreg branch of iio.git.

Note the fixes tag had to be replaced with the full version.
No shortening allowed.

Jonathan

[e-mail] Sept. 11, 2023, 4:31 a.m. UTC | #6

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Jonathan Cameron <[e-mail]>:

On Fri, 30 Jun 2023 22:37:19 +0800 you wrote:
> The struct cros_ec_command contains several integer fields and a
> trailing array. An allocation size neglecting the integer fields can
> lead to buffer overrun.
> 
> Reviewed-by: Tzung-Bi Shih <[e-mail]>
> Signed-off-by: Yiyuan Guo <[e-mail]>
> 
> [...]

Here is the summary with links:
  - [v3] iio: cros_ec: Fix the allocation size for cros_ec_command
    https://git.kernel.org/chrome-platform/c/8a4629055ef5

You are awesome, thank you!

[e-mail] Sept. 11, 2023, 4:49 a.m. UTC | #7

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Jonathan Cameron <[e-mail]>:

On Fri, 30 Jun 2023 22:37:19 +0800 you wrote:
> The struct cros_ec_command contains several integer fields and a
> trailing array. An allocation size neglecting the integer fields can
> lead to buffer overrun.
> 
> Reviewed-by: Tzung-Bi Shih <[e-mail]>
> Signed-off-by: Yiyuan Guo <[e-mail]>
> 
> [...]

Here is the summary with links:
  - [v3] iio: cros_ec: Fix the allocation size for cros_ec_command
    https://git.kernel.org/chrome-platform/c/8a4629055ef5

You are awesome, thank you!

Patch

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 943e9e14d1e9..b72d39fc2434 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -253,7 +253,7 @@  int cros_ec_sensors_core_init(struct platform_device *pdev,
 	platform_set_drvdata(pdev, indio_dev);
 
 	state->ec = ec->ec_dev;
-	state->msg = devm_kzalloc(&pdev->dev,
+	state->msg = devm_kzalloc(&pdev->dev, sizeof(*state->msg) +
 				max((u16)sizeof(struct ec_params_motion_sense),
 				state->ec->max_response), GFP_KERNEL);
 	if (!state->msg)