| Message ID | 20190109055724.184692-1-pihsun@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | thermal: mtk: Allocate enough space for mtk_thermal. | expand |
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 >
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 >>
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 >
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>
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 > >
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 */ >
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 */
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(-)