Patchwork thermal: mtk: Allocate enough space for mtk_thermal.

login
register
mail settings
Submitter Peter Shih
Date Jan. 9, 2019, 5:57 a.m.
Message ID <20190109055724.184692-1-pihsun@chromium.org>
Download mbox | patch
Permalink /patch/695455/
State New
Headers show

Comments

Peter Shih - Jan. 9, 2019, 5:57 a.m.
The mtk_thermal struct contains a 'struct mtk_thermal_bank banks[];',
but the allocation only allocates sizeof(struct mtk_thermal) bytes,
which cause out of bound access with the ->banks[] member. Change it to
a fixed size array instead.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 drivers/thermal/mtk_thermal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Peter Shih - Jan. 30, 2019, 6:04 a.m.
Adding Michael Kao to cc list.

On Wed, Jan 9, 2019 at 1:57 PM Pi-Hsun Shih <pihsun@chromium.org> wrote:
>
> The mtk_thermal struct contains a 'struct mtk_thermal_bank banks[];',
> but the allocation only allocates sizeof(struct mtk_thermal) bytes,
> which cause out of bound access with the ->banks[] member. Change it to
> a fixed size array instead.
>
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
>  drivers/thermal/mtk_thermal.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> index 0691f260f6eabe..ea11edb3fcced6 100644
> --- a/drivers/thermal/mtk_thermal.c
> +++ b/drivers/thermal/mtk_thermal.c
> @@ -159,6 +159,9 @@
>  #define MT7622_NUM_SENSORS_PER_ZONE    1
>  #define MT7622_TS1     0
>
> +/* The maximum number of banks */
> +#define MAX_NUM_ZONES          8
> +
>  struct mtk_thermal;
>
>  struct thermal_bank_cfg {
> @@ -178,7 +181,7 @@ struct mtk_thermal_data {
>         const int *sensor_mux_values;
>         const int *msr;
>         const int *adcpnp;
> -       struct thermal_bank_cfg bank_data[];
> +       struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
>  };
>
>  struct mtk_thermal {
> @@ -197,7 +200,7 @@ struct mtk_thermal {
>         s32 vts[MT8173_NUM_SENSORS];
>
>         const struct mtk_thermal_data *conf;
> -       struct mtk_thermal_bank banks[];
> +       struct mtk_thermal_bank banks[MAX_NUM_ZONES];
>  };
>
>  /* MT8173 thermal sensor data */
> --
> 2.20.1.97.g81188d93c3-goog
>
Daniel Lezcano - Jan. 30, 2019, 7:44 a.m.
On 30/01/2019 07:04, Peter Shih wrote:
> Adding Michael Kao to cc list.
> 
> On Wed, Jan 9, 2019 at 1:57 PM Pi-Hsun Shih <pihsun@chromium.org> wrote:
>>
>> The mtk_thermal struct contains a 'struct mtk_thermal_bank banks[];',
>> but the allocation only allocates sizeof(struct mtk_thermal) bytes,
>> which cause out of bound access with the ->banks[] member. Change it to
>> a fixed size array instead.

Even if the fix is correct, it pushes back the bug later in time if a
new board containing more than MAX_NUM_ZONES is introduced. I suggest to
dynamically allocate the array with the 'num_banks' value.


>> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
>> ---
>>  drivers/thermal/mtk_thermal.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
>> index 0691f260f6eabe..ea11edb3fcced6 100644
>> --- a/drivers/thermal/mtk_thermal.c
>> +++ b/drivers/thermal/mtk_thermal.c
>> @@ -159,6 +159,9 @@
>>  #define MT7622_NUM_SENSORS_PER_ZONE    1
>>  #define MT7622_TS1     0
>>
>> +/* The maximum number of banks */
>> +#define MAX_NUM_ZONES          8
>> +
>>  struct mtk_thermal;
>>
>>  struct thermal_bank_cfg {
>> @@ -178,7 +181,7 @@ struct mtk_thermal_data {
>>         const int *sensor_mux_values;
>>         const int *msr;
>>         const int *adcpnp;
>> -       struct thermal_bank_cfg bank_data[];
>> +       struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
>>  };
>>
>>  struct mtk_thermal {
>> @@ -197,7 +200,7 @@ struct mtk_thermal {
>>         s32 vts[MT8173_NUM_SENSORS];
>>
>>         const struct mtk_thermal_data *conf;
>> -       struct mtk_thermal_bank banks[];
>> +       struct mtk_thermal_bank banks[MAX_NUM_ZONES];
>>  };
>>
>>  /* MT8173 thermal sensor data */
>> --
>> 2.20.1.97.g81188d93c3-goog
>>
Peter Shih - Jan. 30, 2019, 9:25 a.m.
On Wed, Jan 30, 2019 at 3:44 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 30/01/2019 07:04, Peter Shih wrote:
> > Adding Michael Kao to cc list.
> >
> > On Wed, Jan 9, 2019 at 1:57 PM Pi-Hsun Shih <pihsun@chromium.org> wrote:
> >>
> >> The mtk_thermal struct contains a 'struct mtk_thermal_bank banks[];',
> >> but the allocation only allocates sizeof(struct mtk_thermal) bytes,
> >> which cause out of bound access with the ->banks[] member. Change it to
> >> a fixed size array instead.
>
> Even if the fix is correct, it pushes back the bug later in time if a
> new board containing more than MAX_NUM_ZONES is introduced. I suggest to
> dynamically allocate the array with the 'num_banks' value.
>

For the current code structure, those mtk_thermal_data are statically declared,
so if there's new board containing more than MAX_NUM_ZONES of bank_data, it
would actually be a compile error.

I'm fine with either way, but feel like that this is simpler than manually
calculating the size needed for allocation.

>
> >> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> >> ---
> >>  drivers/thermal/mtk_thermal.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> >> index 0691f260f6eabe..ea11edb3fcced6 100644
> >> --- a/drivers/thermal/mtk_thermal.c
> >> +++ b/drivers/thermal/mtk_thermal.c
> >> @@ -159,6 +159,9 @@
> >>  #define MT7622_NUM_SENSORS_PER_ZONE    1
> >>  #define MT7622_TS1     0
> >>
> >> +/* The maximum number of banks */
> >> +#define MAX_NUM_ZONES          8
> >> +
> >>  struct mtk_thermal;
> >>
> >>  struct thermal_bank_cfg {
> >> @@ -178,7 +181,7 @@ struct mtk_thermal_data {
> >>         const int *sensor_mux_values;
> >>         const int *msr;
> >>         const int *adcpnp;
> >> -       struct thermal_bank_cfg bank_data[];
> >> +       struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
> >>  };
> >>
> >>  struct mtk_thermal {
> >> @@ -197,7 +200,7 @@ struct mtk_thermal {
> >>         s32 vts[MT8173_NUM_SENSORS];
> >>
> >>         const struct mtk_thermal_data *conf;
> >> -       struct mtk_thermal_bank banks[];
> >> +       struct mtk_thermal_bank banks[MAX_NUM_ZONES];
> >>  };
> >>
> >>  /* MT8173 thermal sensor data */
> >> --
> >> 2.20.1.97.g81188d93c3-goog
> >>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
Daniel Lezcano - Jan. 30, 2019, 10:04 a.m.
On 30/01/2019 10:25, Pi-Hsun Shih wrote:
> On Wed, Jan 30, 2019 at 3:44 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 30/01/2019 07:04, Peter Shih wrote:
>>> Adding Michael Kao to cc list.
>>>
>>> On Wed, Jan 9, 2019 at 1:57 PM Pi-Hsun Shih <pihsun@chromium.org> wrote:
>>>>
>>>> The mtk_thermal struct contains a 'struct mtk_thermal_bank banks[];',
>>>> but the allocation only allocates sizeof(struct mtk_thermal) bytes,
>>>> which cause out of bound access with the ->banks[] member. Change it to
>>>> a fixed size array instead.
>>
>> Even if the fix is correct, it pushes back the bug later in time if a
>> new board containing more than MAX_NUM_ZONES is introduced. I suggest to
>> dynamically allocate the array with the 'num_banks' value.
>>
> 
> For the current code structure, those mtk_thermal_data are statically declared,
> so if there's new board containing more than MAX_NUM_ZONES of bank_data, it
> would actually be a compile error.
> 
> I'm fine with either way, but feel like that this is simpler than manually
> calculating the size needed for allocation.

Right, I missed it can be caught at compile time.

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Zhang, Rui - Jan. 30, 2019, 1:38 p.m.
On 三, 2019-01-30 at 11:04 +0100, Daniel Lezcano wrote:
> On 30/01/2019 10:25, Pi-Hsun Shih wrote:
> > 
> > On Wed, Jan 30, 2019 at 3:44 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> > > 
> > > 
> > > On 30/01/2019 07:04, Peter Shih wrote:
> > > > 
> > > > Adding Michael Kao to cc list.
> > > > 
> > > > On Wed, Jan 9, 2019 at 1:57 PM Pi-Hsun Shih <pihsun@chromium.or
> > > > g> wrote:
> > > > > 
> > > > > 
> > > > > The mtk_thermal struct contains a 'struct mtk_thermal_bank
> > > > > banks[];',
> > > > > but the allocation only allocates sizeof(struct mtk_thermal)
> > > > > bytes,
> > > > > which cause out of bound access with the ->banks[] member.
> > > > > Change it to
> > > > > a fixed size array instead.
> > > Even if the fix is correct, it pushes back the bug later in time
> > > if a
> > > new board containing more than MAX_NUM_ZONES is introduced. I
> > > suggest to
> > > dynamically allocate the array with the 'num_banks' value.
> > > 
> > For the current code structure, those mtk_thermal_data are
> > statically declared,
> > so if there's new board containing more than MAX_NUM_ZONES of
> > bank_data, it
> > would actually be a compile error.
> > 
> > I'm fine with either way, but feel like that this is simpler than
> > manually
> > calculating the size needed for allocation.
> Right, I missed it can be caught at compile time.
> 
> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
As this is a bugfix, I will take it and queue it for next -rc.

thanks,
rui
> 
>
Matthias Brugger - Feb. 7, 2019, 4:25 p.m.
On 09/01/2019 06:57, Pi-Hsun Shih wrote:
> The mtk_thermal struct contains a 'struct mtk_thermal_bank banks[];',
> but the allocation only allocates sizeof(struct mtk_thermal) bytes,
> which cause out of bound access with the ->banks[] member. Change it to
> a fixed size array instead.
> 
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>

For the next time, please don't forget to provide a fixes tag so that it can get
into stable trees automatically.

For the records, it fixes commit (v4.9):
b7cf0053738c ("thermal: Add Mediatek thermal driver for mt2701.")


> ---
>  drivers/thermal/mtk_thermal.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> index 0691f260f6eabe..ea11edb3fcced6 100644
> --- a/drivers/thermal/mtk_thermal.c
> +++ b/drivers/thermal/mtk_thermal.c
> @@ -159,6 +159,9 @@
>  #define MT7622_NUM_SENSORS_PER_ZONE	1
>  #define MT7622_TS1	0
>  
> +/* The maximum number of banks */
> +#define MAX_NUM_ZONES		8
> +
>  struct mtk_thermal;
>  
>  struct thermal_bank_cfg {
> @@ -178,7 +181,7 @@ struct mtk_thermal_data {
>  	const int *sensor_mux_values;
>  	const int *msr;
>  	const int *adcpnp;
> -	struct thermal_bank_cfg bank_data[];
> +	struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
>  };
>  
>  struct mtk_thermal {
> @@ -197,7 +200,7 @@ struct mtk_thermal {
>  	s32 vts[MT8173_NUM_SENSORS];
>  
>  	const struct mtk_thermal_data *conf;
> -	struct mtk_thermal_bank banks[];
> +	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
>  };
>  
>  /* MT8173 thermal sensor data */
>

Patch

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index 0691f260f6eabe..ea11edb3fcced6 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -159,6 +159,9 @@ 
 #define MT7622_NUM_SENSORS_PER_ZONE	1
 #define MT7622_TS1	0
 
+/* The maximum number of banks */
+#define MAX_NUM_ZONES		8
+
 struct mtk_thermal;
 
 struct thermal_bank_cfg {
@@ -178,7 +181,7 @@  struct mtk_thermal_data {
 	const int *sensor_mux_values;
 	const int *msr;
 	const int *adcpnp;
-	struct thermal_bank_cfg bank_data[];
+	struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
 };
 
 struct mtk_thermal {
@@ -197,7 +200,7 @@  struct mtk_thermal {
 	s32 vts[MT8173_NUM_SENSORS];
 
 	const struct mtk_thermal_data *conf;
-	struct mtk_thermal_bank banks[];
+	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
 };
 
 /* MT8173 thermal sensor data */