[PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[Manivannan Sadhasivam]

In "qmp_cooling_devices_register", the count value is initially
QMP_NUM_COOLING_RESOURCES, which is 2. Based on the initial count value,
the memory for cooling_devs is allocated. Then while calling the
"qmp_cooling_device_add" function, count value is post-incremented for
each child node.

This makes the out of bound access to the cooling_dev array. Fix it by
resetting the count value to zero before adding cooling devices.

While at it, let's also free the memory allocated to cooling_dev if no
cooling device is found in DT and during unroll phase.

Cc: stable@vger.kernel.org # 5.4
Fixes: 05589b30b21a ("soc: qcom: Extend AOSS QMP driver to support resources that are used to wake up the SoC.")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

Bjorn: I've just compile tested this patch.

 drivers/soc/qcom/qcom_aoss.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 934fcc4d2b05..98c665411768 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -488,6 +488,7 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
 	if (!qmp->cooling_devs)
 		return -ENOMEM;
 
+	count = 0;
 	for_each_available_child_of_node(np, child) {
 		if (!of_find_property(child, "#cooling-cells", NULL))
 			continue;
@@ -497,12 +498,16 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
 			goto unroll;
 	}
 
+	if (!count)
+		devm_kfree(qmp->dev, qmp->cooling_devs);
+
 	return 0;
 
 unroll:
 	while (--count >= 0)
 		thermal_cooling_device_unregister
 			(qmp->cooling_devs[count].cdev);
+	devm_kfree(qmp->dev, qmp->cooling_devs);
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[Matthias Kaehlcke]

On Mon, Jun 28, 2021 at 10:57:41PM +0530, Manivannan Sadhasivam wrote:
> In "qmp_cooling_devices_register", the count value is initially
> QMP_NUM_COOLING_RESOURCES, which is 2. Based on the initial count value,
> the memory for cooling_devs is allocated. Then while calling the
> "qmp_cooling_device_add" function, count value is post-incremented for
> each child node.
> 
> This makes the out of bound access to the cooling_dev array. Fix it by
> resetting the count value to zero before adding cooling devices.
> 
> While at it, let's also free the memory allocated to cooling_dev if no
> cooling device is found in DT and during unroll phase.
> 
> Cc: stable@vger.kernel.org # 5.4
> Fixes: 05589b30b21a ("soc: qcom: Extend AOSS QMP driver to support resources that are used to wake up the SoC.")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> 
> Bjorn: I've just compile tested this patch.
> 
>  drivers/soc/qcom/qcom_aoss.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 934fcc4d2b05..98c665411768 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -488,6 +488,7 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
>  	if (!qmp->cooling_devs)
>  		return -ENOMEM;
>  
> +	count = 0;
>  	for_each_available_child_of_node(np, child) {
>  		if (!of_find_property(child, "#cooling-cells", NULL))
>  			continue;
> @@ -497,12 +498,16 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
>  			goto unroll;
>  	}
>  
> +	if (!count)
> +		devm_kfree(qmp->dev, qmp->cooling_devs);
> +
>  	return 0;
>  
>  unroll:
>  	while (--count >= 0)
>  		thermal_cooling_device_unregister
>  			(qmp->cooling_devs[count].cdev);
> +	devm_kfree(qmp->dev, qmp->cooling_devs);
>  
>  	return ret;
>  }


A few more previous lines of code for context:

  int count = QMP_NUM_COOLING_RESOURCES;

  qmp->cooling_devs = devm_kcalloc(qmp->dev, count,
                                   sizeof(*qmp->cooling_devs),
                                   GFP_KERNEL);

I would suggest to initialize 'count' to 0 from the start and pass
QMP_NUM_COOLING_RESOURCES to devm_kcalloc() rather than 'count',
instead of resetting 'count' afterwards.

Re: [PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[Manivannan Sadhasivam]

On Mon, Jun 28, 2021 at 04:03:14PM -0700, Matthias Kaehlcke wrote:

[...]

> 
> 
> A few more previous lines of code for context:
> 
>   int count = QMP_NUM_COOLING_RESOURCES;
> 
>   qmp->cooling_devs = devm_kcalloc(qmp->dev, count,
>                                    sizeof(*qmp->cooling_devs),
>                                    GFP_KERNEL);
> 
> I would suggest to initialize 'count' to 0 from the start and pass
> QMP_NUM_COOLING_RESOURCES to devm_kcalloc() rather than 'count',
> instead of resetting 'count' afterwards.

Yeah, I thought about it but the actual bug in the code is not resetting
the count value to 0. So fixing this way seems a better option.

Thanks,
Mani

Re: [PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[Matthias Kaehlcke]

On Tue, Jun 29, 2021 at 09:55:58AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 28, 2021 at 04:03:14PM -0700, Matthias Kaehlcke wrote:
> 
> [...]
> 
> > 
> > 
> > A few more previous lines of code for context:
> > 
> >   int count = QMP_NUM_COOLING_RESOURCES;
> > 
> >   qmp->cooling_devs = devm_kcalloc(qmp->dev, count,
> >                                    sizeof(*qmp->cooling_devs),
> >                                    GFP_KERNEL);
> > 
> > I would suggest to initialize 'count' to 0 from the start and pass
> > QMP_NUM_COOLING_RESOURCES to devm_kcalloc() rather than 'count',
> > instead of resetting 'count' afterwards.
> 
> Yeah, I thought about it but the actual bug in the code is not resetting
> the count value to 0. So fixing this way seems a better option.

I don't agree that it's the better option. IMO it's clearer to pass
the constant QMP_NUM_COOLING_RESOURCES directly to devm_kcalloc(),
rather than giving the impression that the number of allocated items
is variable. Repurposing variables can be confusing and led to this
bug. Also the resulting code doesn't need to re-initialize 'count'.

Re: [PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[Manivannan Sadhasivam]

On Tue, Jun 29, 2021 at 07:17:20AM -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 29, 2021 at 09:55:58AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jun 28, 2021 at 04:03:14PM -0700, Matthias Kaehlcke wrote:
> > 
> > [...]
> > 
> > > 
> > > 
> > > A few more previous lines of code for context:
> > > 
> > >   int count = QMP_NUM_COOLING_RESOURCES;
> > > 
> > >   qmp->cooling_devs = devm_kcalloc(qmp->dev, count,
> > >                                    sizeof(*qmp->cooling_devs),
> > >                                    GFP_KERNEL);
> > > 
> > > I would suggest to initialize 'count' to 0 from the start and pass
> > > QMP_NUM_COOLING_RESOURCES to devm_kcalloc() rather than 'count',
> > > instead of resetting 'count' afterwards.
> > 
> > Yeah, I thought about it but the actual bug in the code is not resetting
> > the count value to 0. So fixing this way seems a better option.
> 
> I don't agree that it's the better option. IMO it's clearer to pass
> the constant QMP_NUM_COOLING_RESOURCES directly to devm_kcalloc(),
> rather than giving the impression that the number of allocated items
> is variable. Repurposing variables can be confusing and led to this
> bug. Also the resulting code doesn't need to re-initialize 'count'.

I don't dis-agree with you on this :) Let me send v2 incorporating the
comments.

Thanks,
Mani

Re: [PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[Bjorn Andersson]

On Mon 28 Jun 12:27 CDT 2021, Manivannan Sadhasivam wrote:

> In "qmp_cooling_devices_register", the count value is initially
> QMP_NUM_COOLING_RESOURCES, which is 2. Based on the initial count value,
> the memory for cooling_devs is allocated. Then while calling the
> "qmp_cooling_device_add" function, count value is post-incremented for
> each child node.
> 
> This makes the out of bound access to the cooling_dev array. Fix it by
> resetting the count value to zero before adding cooling devices.
> 
> While at it, let's also free the memory allocated to cooling_dev if no
> cooling device is found in DT and during unroll phase.
> 
> Cc: stable@vger.kernel.org # 5.4
> Fixes: 05589b30b21a ("soc: qcom: Extend AOSS QMP driver to support resources that are used to wake up the SoC.")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> 
> Bjorn: I've just compile tested this patch.
> 
>  drivers/soc/qcom/qcom_aoss.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 934fcc4d2b05..98c665411768 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -488,6 +488,7 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
>  	if (!qmp->cooling_devs)
>  		return -ENOMEM;
>  
> +	count = 0;

This will address the immediate problem, which is that we assign
cooling_devs[2..] in this loop. But, like Matthias I don't think we
should use "count" as a constant in the first hunk and then reset it
and use it as a counter in the second hunk.

Frankly, I don't see why cooling_devs is dynamically allocated (without
being conditional on there being any children).


So, could you please make the cooling_devs an array of size
QMP_NUM_COOLING_RESOURCES, remove the dynamic allocation here, just
initialize count to 0 and add a check in the loop to generate an error
if count == QMP_NUM_COOLING_RESOURCES?

>  	for_each_available_child_of_node(np, child) {
>  		if (!of_find_property(child, "#cooling-cells", NULL))
>  			continue;
> @@ -497,12 +498,16 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
>  			goto unroll;
>  	}
>  
> +	if (!count)
> +		devm_kfree(qmp->dev, qmp->cooling_devs);

I presume this is just an optimization, to get some memory back when
there's no cooling devices specified in DT.  I don't think this is
necessary and this made me want the static sizing of the array..

> +
>  	return 0;
>  
>  unroll:
>  	while (--count >= 0)
>  		thermal_cooling_device_unregister
>  			(qmp->cooling_devs[count].cdev);
> +	devm_kfree(qmp->dev, qmp->cooling_devs);

I don't remember why we don't fail probe() when this happens, seems like
the DT properties should be optional but the errors adding them should
be fatal. But that's a separate issue.

Regards,
Bjorn

>  
>  	return ret;
>  }
> -- 
> 2.25.1
> 

RE: [PATCH] soc: qcom: aoss: Fix the out of bound usage of cooling_devs

[David Laight]

From: Bjorn Andersson
> Sent: 29 June 2021 16:44
> 
> On Mon 28 Jun 12:27 CDT 2021, Manivannan Sadhasivam wrote:
> 
> > In "qmp_cooling_devices_register", the count value is initially
> > QMP_NUM_COOLING_RESOURCES, which is 2. Based on the initial count value,
> > the memory for cooling_devs is allocated. Then while calling the
> > "qmp_cooling_device_add" function, count value is post-incremented for
> > each child node.
> >
> > This makes the out of bound access to the cooling_dev array. Fix it by
> > resetting the count value to zero before adding cooling devices.
> >
> > While at it, let's also free the memory allocated to cooling_dev if no
> > cooling device is found in DT and during unroll phase.
> >
> > Cc: stable@vger.kernel.org # 5.4
> > Fixes: 05589b30b21a ("soc: qcom: Extend AOSS QMP driver to support resources that are used to wake
> up the SoC.")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >
> > Bjorn: I've just compile tested this patch.
> >
> >  drivers/soc/qcom/qcom_aoss.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> > index 934fcc4d2b05..98c665411768 100644
> > --- a/drivers/soc/qcom/qcom_aoss.c
> > +++ b/drivers/soc/qcom/qcom_aoss.c
> > @@ -488,6 +488,7 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
> >  	if (!qmp->cooling_devs)
> >  		return -ENOMEM;
> >
> > +	count = 0;
> 
> This will address the immediate problem, which is that we assign
> cooling_devs[2..] in this loop. But, like Matthias I don't think we
> should use "count" as a constant in the first hunk and then reset it
> and use it as a counter in the second hunk.
> 
> Frankly, I don't see why cooling_devs is dynamically allocated (without
> being conditional on there being any children).

Not to mention what happens if there are 3 matching nodes.
Given that qmp_cooling_device is only 4 pointers why bother
allocating a pointer to it at all.
Just instantiate 2 of them in the outer structure.
No is going to notice 56 bytes - it is probably hidden in the
padding when the outer structure is allocated.


> So, could you please make the cooling_devs an array of size
> QMP_NUM_COOLING_RESOURCES, remove the dynamic allocation here, just
> initialize count to 0 and add a check in the loop to generate an error
> if count == QMP_NUM_COOLING_RESOURCES?
> 

You should set count to 0 just before this loop - not at the top
of the function.

> >  	for_each_available_child_of_node(np, child) {
> >  		if (!of_find_property(child, "#cooling-cells", NULL))
> >  			continue;
> > @@ -497,12 +498,16 @@ static int qmp_cooling_devices_register(struct qmp *qmp)
> >  			goto unroll;
> >  	}
> >
> > +	if (!count)
> > +		devm_kfree(qmp->dev, qmp->cooling_devs);
> 
> I presume this is just an optimization, to get some memory back when
> there's no cooling devices specified in DT.  I don't think this is
> necessary and this made me want the static sizing of the array..
> 
> > +
> >  	return 0;
> >
> >  unroll:
> >  	while (--count >= 0)
> >  		thermal_cooling_device_unregister
> >  			(qmp->cooling_devs[count].cdev);
> > +	devm_kfree(qmp->dev, qmp->cooling_devs);
> 
> I don't remember why we don't fail probe() when this happens, seems like
> the DT properties should be optional but the errors adding them should
> be fatal. But that's a separate issue.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)