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

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

[Yiyuan Guo]

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 <tzungbi@kernel.org>
Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
---
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)
-- 
2.25.1

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

[Jonathan Cameron]

On Fri, 30 Jun 2023 22:37:19 +0800
Yiyuan Guo <yguoaz@gmail.com> 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 <tzungbi@kernel.org>
> Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>

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)

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

[yguoaz]

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 <jic23@kernel.org> wrote:
>
> On Fri, 30 Jun 2023 22:37:19 +0800
> Yiyuan Guo <yguoaz@gmail.com> 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 <tzungbi@kernel.org>
> > Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
>
> 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)
>

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

[Tzung-Bi Shih]

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.

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

[Jonathan Cameron]

On Mon, 17 Jul 2023 11:09:08 +0800
Tzung-Bi Shih <tzungbi@kernel.org> 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.

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

[Jonathan Cameron]

On Tue, 18 Jul 2023 10:37:02 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 17 Jul 2023 11:09:08 +0800
> Tzung-Bi Shih <tzungbi@kernel.org> 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

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

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Jonathan Cameron <Jonathan.Cameron@huawei.com>:

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 <tzungbi@kernel.org>
> Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
> 
> [...]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Jonathan Cameron <Jonathan.Cameron@huawei.com>:

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 <tzungbi@kernel.org>
> Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
> 
> [...]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2] iio: cros_ec: Fix the allocation size for cros_ec_command

[Tzung-Bi Shih]

On Fri, Jun 30, 2023 at 02:31:32PM +0800, Yiyuan Guo 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.
> 
> Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>

You could attach my R-b tag as v2 has no major changes from v1.

> ---

Always a good practice to put changelog here.  Search "changelog" in [1].

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> @@ -253,8 +253,8 @@ 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,
> -				max((u16)sizeof(struct ec_params_motion_sense),
> +	state->msg = devm_kzalloc(&pdev->dev, sizeof(*state->msg) +
> +			max((u16)sizeof(struct ec_params_motion_sense),
>  				state->ec->max_response), GFP_KERNEL);

While looking at the patch again, I found a nit.  Please align the code by
adding an extra tab before "max".

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

[Yiyuan Guo]

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 <tzungbi@kernel.org>
Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
---
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)
-- 
2.25.1

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

[Guenter Roeck]

On Fri, Jun 30, 2023 at 1:31 AM Yiyuan Guo <yguoaz@gmail.com> 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 <tzungbi@kernel.org>
> Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>

Please _never_ send a patch as reply to a previous one, much less with
a Re: subject.

Guenter

> ---
> 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)
> --
> 2.25.1
>

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

[yguoaz]

Got it. I have resent the patch in a separate thread.

Thanks,
Yiyuan

On Fri, Jun 30, 2023 at 10:07 PM Guenter Roeck <groeck@google.com> wrote:
>
> On Fri, Jun 30, 2023 at 1:31 AM Yiyuan Guo <yguoaz@gmail.com> 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 <tzungbi@kernel.org>
> > Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
>
> Please _never_ send a patch as reply to a previous one, much less with
> a Re: subject.
>
> Guenter
>
> > ---
> > 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)
> > --
> > 2.25.1
> >