2019-01-09 06:46:05

by Pi-Hsun Shih

[permalink] [raw]
Subject: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.

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 <[email protected]>
---
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



2019-01-30 06:05:15

by Pi-Hsun Shih

[permalink] [raw]
Subject: Re: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.

Adding Michael Kao to cc list.

On Wed, Jan 9, 2019 at 1:57 PM Pi-Hsun Shih <[email protected]> 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 <[email protected]>
> ---
> 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
>

2019-01-30 07:45:32

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.

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 <[email protected]> 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 <[email protected]>
>> ---
>> 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


2019-01-30 09:26:20

by Pi-Hsun Shih

[permalink] [raw]
Subject: Re: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.

On Wed, Jan 30, 2019 at 3:44 PM Daniel Lezcano
<[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
>

2019-01-30 10:05:47

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.

On 30/01/2019 10:25, Pi-Hsun Shih wrote:
> On Wed, Jan 30, 2019 at 3:44 PM Daniel Lezcano
> <[email protected]> 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 <[email protected]> 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 <[email protected]>



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


2019-01-30 13:39:07

by Zhang, Rui

[permalink] [raw]
Subject: Re: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.

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
> > <[email protected]> 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 <[email protected]
> > > > 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 <[email protected]>
>
As this is a bugfix, I will take it and queue it for next -rc.

thanks,
rui
>
>

2019-02-07 16:27:34

by Matthias Brugger

[permalink] [raw]
Subject: Re: [PATCH] thermal: mtk: Allocate enough space for mtk_thermal.



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 <[email protected]>

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 */
>