| Message ID | 20210628172741.16894-1-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | soc: qcom: aoss: Fix the out of bound usage of cooling_devs | expand |
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.
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
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'.
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
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 >
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; }
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(+) -- 2.25.1