| Message ID | 1453998832-27383-3-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> wrote: > The PMC base address pointer is initialised during early boot so that > early platform code may used the PMC public functions. During the probe > of the PMC driver the base address pointer is mapped again and the initial > mapping is freed. This exposes a window where a device accessing the PMC > registers via one of the public functions, could race with the updating > of the pointer and lead to a invalid access. Furthermore, the only > protection between multiple devices attempting to access the PMC registers > is when setting the powergate state to on or off. None of the other public > functions that access the PMC registers are protected. > > Use the existing mutex to protect paths that may race with regard to > accessing the PMC registers. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 35 insertions(+), 9 deletions(-) > > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c > index 85b4e166273a..f8cdb7ce9755 100644 > --- a/drivers/soc/tegra/pmc.c > +++ b/drivers/soc/tegra/pmc.c > @@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id) > if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) > return -EINVAL; > > + mutex_lock(&pmc->powergates_lock); > status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id); > + mutex_unlock(&pmc->powergates_lock); > + > return !!status; > } > > @@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id) > if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) > return -EINVAL; > > + mutex_lock(&pmc->powergates_lock); > + > /* > * On Tegra124 and later, the clamps for the GPU are controlled by a > * separate register (with different semantics). > @@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id) > if (id == TEGRA_POWERGATE_3D) { > if (pmc->soc->has_gpu_clamps) { > tegra_pmc_writel(0, GPU_RG_CNTRL); > - return 0; > + goto out; > } > } > > @@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id) > > tegra_pmc_writel(mask, REMOVE_CLAMPING); > > +out: > + mutex_unlock(&pmc->powergates_lock); > + > return 0; > } > EXPORT_SYMBOL(tegra_powergate_remove_clamping); > @@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id) > unsigned int bit, mask; > int err; > > + mutex_lock(&pmc->powergates_lock); > + > err = tegra_io_rail_prepare(id, &request, &status, &bit); > if (err < 0) > - return err; > + goto error; > > mask = 1 << bit; > > @@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id) > err = tegra_io_rail_poll(status, mask, 0, 250); > if (err < 0) { > pr_info("tegra_io_rail_poll() failed: %d\n", err); > - return err; > + goto error; > } > > tegra_io_rail_unprepare(); > > - return 0; > +error: > + mutex_unlock(&pmc->powergates_lock); > + > + return err < 0 ? err : 0; Is this necessary? Why simply not returning 'err'? From what I see 'tegra_io_rail_power_on()' can only return a negative value or '0'. > } > EXPORT_SYMBOL(tegra_io_rail_power_on); > > @@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id) > unsigned int bit, mask; > int err; > > + mutex_lock(&pmc->powergates_lock); > + > err = tegra_io_rail_prepare(id, &request, &status, &bit); > if (err < 0) { > pr_info("tegra_io_rail_prepare() failed: %d\n", err); > - return err; > + goto error; > } > > mask = 1 << bit; > @@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id) > > err = tegra_io_rail_poll(status, mask, mask, 250); > if (err < 0) > - return err; > + goto error; > > tegra_io_rail_unprepare(); > > - return 0; > +error: > + mutex_unlock(&pmc->powergates_lock); > + > + return err < 0 ? err : 0; Same comment as above. > } > EXPORT_SYMBOL(tegra_io_rail_power_off); > > @@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev) > > /* take over the memory region from the early initialization */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + mutex_lock(&pmc->powergates_lock); > pmc->base = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(pmc->base)) > - return PTR_ERR(pmc->base); > + mutex_unlock(&pmc->powergates_lock); Since the mutex is released there is a window of opportunity for devices to access an erroneous pointer. A better approach might be to use a temporary variable, do all the initialisation that is required and when things look good set pmc-base to that temporary variable. Mathieu > + > + if (IS_ERR(pmc->base)) { > + err = PTR_ERR(pmc->base); > + goto error; > + } > > pmc->clk = devm_clk_get(&pdev->dev, "pclk"); > if (IS_ERR(pmc->clk)) { > @@ -853,7 +877,9 @@ static int tegra_pmc_probe(struct platform_device *pdev) > return 0; > > error: > + mutex_lock(&pmc->powergates_lock); > pmc->base = base; > + mutex_unlock(&pmc->powergates_lock); > > return err; > } > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/01/16 16:20, Mathieu Poirier wrote: > On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> wrote: >> The PMC base address pointer is initialised during early boot so that >> early platform code may used the PMC public functions. During the probe >> of the PMC driver the base address pointer is mapped again and the initial >> mapping is freed. This exposes a window where a device accessing the PMC >> registers via one of the public functions, could race with the updating >> of the pointer and lead to a invalid access. Furthermore, the only >> protection between multiple devices attempting to access the PMC registers >> is when setting the powergate state to on or off. None of the other public >> functions that access the PMC registers are protected. >> >> Use the existing mutex to protect paths that may race with regard to >> accessing the PMC registers. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 35 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c >> index 85b4e166273a..f8cdb7ce9755 100644 >> --- a/drivers/soc/tegra/pmc.c >> +++ b/drivers/soc/tegra/pmc.c >> @@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id) >> if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) >> return -EINVAL; >> >> + mutex_lock(&pmc->powergates_lock); >> status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id); >> + mutex_unlock(&pmc->powergates_lock); >> + >> return !!status; >> } >> >> @@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id) >> if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) >> return -EINVAL; >> >> + mutex_lock(&pmc->powergates_lock); >> + >> /* >> * On Tegra124 and later, the clamps for the GPU are controlled by a >> * separate register (with different semantics). >> @@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id) >> if (id == TEGRA_POWERGATE_3D) { >> if (pmc->soc->has_gpu_clamps) { >> tegra_pmc_writel(0, GPU_RG_CNTRL); >> - return 0; >> + goto out; >> } >> } >> >> @@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id) >> >> tegra_pmc_writel(mask, REMOVE_CLAMPING); >> >> +out: >> + mutex_unlock(&pmc->powergates_lock); >> + >> return 0; >> } >> EXPORT_SYMBOL(tegra_powergate_remove_clamping); >> @@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id) >> unsigned int bit, mask; >> int err; >> >> + mutex_lock(&pmc->powergates_lock); >> + >> err = tegra_io_rail_prepare(id, &request, &status, &bit); >> if (err < 0) >> - return err; >> + goto error; >> >> mask = 1 << bit; >> >> @@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id) >> err = tegra_io_rail_poll(status, mask, 0, 250); >> if (err < 0) { >> pr_info("tegra_io_rail_poll() failed: %d\n", err); >> - return err; >> + goto error; >> } >> >> tegra_io_rail_unprepare(); >> >> - return 0; >> +error: >> + mutex_unlock(&pmc->powergates_lock); >> + >> + return err < 0 ? err : 0; > > Is this necessary? Why simply not returning 'err'? From what I see > 'tegra_io_rail_power_on()' can only return a negative value or '0'. Right, this is probably not necessary and so I could simplify this. >> } >> EXPORT_SYMBOL(tegra_io_rail_power_on); >> >> @@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id) >> unsigned int bit, mask; >> int err; >> >> + mutex_lock(&pmc->powergates_lock); >> + >> err = tegra_io_rail_prepare(id, &request, &status, &bit); >> if (err < 0) { >> pr_info("tegra_io_rail_prepare() failed: %d\n", err); >> - return err; >> + goto error; >> } >> >> mask = 1 << bit; >> @@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id) >> >> err = tegra_io_rail_poll(status, mask, mask, 250); >> if (err < 0) >> - return err; >> + goto error; >> >> tegra_io_rail_unprepare(); >> >> - return 0; >> +error: >> + mutex_unlock(&pmc->powergates_lock); >> + >> + return err < 0 ? err : 0; > > Same comment as above. > >> } >> EXPORT_SYMBOL(tegra_io_rail_power_off); >> >> @@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev) >> >> /* take over the memory region from the early initialization */ >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + >> + mutex_lock(&pmc->powergates_lock); >> pmc->base = devm_ioremap_resource(&pdev->dev, res); >> - if (IS_ERR(pmc->base)) >> - return PTR_ERR(pmc->base); >> + mutex_unlock(&pmc->powergates_lock); > > Since the mutex is released there is a window of opportunity for > devices to access an erroneous pointer. A better approach might be to > use a temporary variable, do all the initialisation that is required > and when things look good set pmc-base to that temporary variable. Thanks. Not sure what I was thinking here. I will fix that. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index 85b4e166273a..f8cdb7ce9755 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id) if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) return -EINVAL; + mutex_lock(&pmc->powergates_lock); status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id); + mutex_unlock(&pmc->powergates_lock); + return !!status; } @@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id) if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) return -EINVAL; + mutex_lock(&pmc->powergates_lock); + /* * On Tegra124 and later, the clamps for the GPU are controlled by a * separate register (with different semantics). @@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id) if (id == TEGRA_POWERGATE_3D) { if (pmc->soc->has_gpu_clamps) { tegra_pmc_writel(0, GPU_RG_CNTRL); - return 0; + goto out; } } @@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id) tegra_pmc_writel(mask, REMOVE_CLAMPING); +out: + mutex_unlock(&pmc->powergates_lock); + return 0; } EXPORT_SYMBOL(tegra_powergate_remove_clamping); @@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id) unsigned int bit, mask; int err; + mutex_lock(&pmc->powergates_lock); + err = tegra_io_rail_prepare(id, &request, &status, &bit); if (err < 0) - return err; + goto error; mask = 1 << bit; @@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id) err = tegra_io_rail_poll(status, mask, 0, 250); if (err < 0) { pr_info("tegra_io_rail_poll() failed: %d\n", err); - return err; + goto error; } tegra_io_rail_unprepare(); - return 0; +error: + mutex_unlock(&pmc->powergates_lock); + + return err < 0 ? err : 0; } EXPORT_SYMBOL(tegra_io_rail_power_on); @@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id) unsigned int bit, mask; int err; + mutex_lock(&pmc->powergates_lock); + err = tegra_io_rail_prepare(id, &request, &status, &bit); if (err < 0) { pr_info("tegra_io_rail_prepare() failed: %d\n", err); - return err; + goto error; } mask = 1 << bit; @@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id) err = tegra_io_rail_poll(status, mask, mask, 250); if (err < 0) - return err; + goto error; tegra_io_rail_unprepare(); - return 0; +error: + mutex_unlock(&pmc->powergates_lock); + + return err < 0 ? err : 0; } EXPORT_SYMBOL(tegra_io_rail_power_off); @@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev) /* take over the memory region from the early initialization */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + mutex_lock(&pmc->powergates_lock); pmc->base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(pmc->base)) - return PTR_ERR(pmc->base); + mutex_unlock(&pmc->powergates_lock); + + if (IS_ERR(pmc->base)) { + err = PTR_ERR(pmc->base); + goto error; + } pmc->clk = devm_clk_get(&pdev->dev, "pclk"); if (IS_ERR(pmc->clk)) { @@ -853,7 +877,9 @@ static int tegra_pmc_probe(struct platform_device *pdev) return 0; error: + mutex_lock(&pmc->powergates_lock); pmc->base = base; + mutex_unlock(&pmc->powergates_lock); return err; }
The PMC base address pointer is initialised during early boot so that early platform code may used the PMC public functions. During the probe of the PMC driver the base address pointer is mapped again and the initial mapping is freed. This exposes a window where a device accessing the PMC registers via one of the public functions, could race with the updating of the pointer and lead to a invalid access. Furthermore, the only protection between multiple devices attempting to access the PMC registers is when setting the powergate state to on or off. None of the other public functions that access the PMC registers are protected. Use the existing mutex to protect paths that may race with regard to accessing the PMC registers. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-)