Messages in this thread | | | From | Chen-Yu Tsai <> | Date | Wed, 19 Jul 2023 16:52:39 +0800 | Subject | Re: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size |
| |
On Wed, Jul 19, 2023 at 4:36 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 19/07/23 10:20, Chen-Yu Tsai ha scritto: > > The MediaTek clock driver library's simple-probe mechanism allocates > > clk_hw_onecell_data based on how many clocks are defined. If there's a > > mismatch between that and the actual number of clocks defined in the DT > > binding, such that a clock ID exceeds the number, then an out-of-bounds > > write will occur. This silently corrupts memory, maybe causing a crash > > later on that seems unrelated. KASAN can detect this if turned on. > > > > To avoid getting into said scenario, check if the to be registered > > clock's ID will fit in the allocated clk_hw_onecell_data. If not, put > > out a big warning and skip the clock. > > > > One could argue that the proper fix is to let the drivers specify the > > number of clocks (based on a DT binding macro) instead. However even > > the DT binding macro could be wrong. And having code to catch errors > > and give out warnings is better than having silent corruption. > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > --- > > This one is less urgent. > > > > Angelo, do you think we should add a field to struct mtk_clk_desc and > > assign CLK_XXX_NR_CLK to it? > > > > I get the point... but I don't know if it is a good idea to add checks for > *bad code* in the first place, as bad code shall not happen at all. > Validating whether a developer wrote the right thing is something that should > be done in code reviews, and such mistakes shouldn't be allowed to happen.
In theory, yeah. In practice, well, we already have such an error in tree. :p
> Besides, if you really want to go this route... In that case I disagree about > the `continue`, as I would be inflexible: if your code is bad, I will refuse > to register your clocks entirely. > That'll force the developer to actually fix it, as parts of the SoC and/or > platform will *not work at all* :-) > > So, in that case... > > if (rc->id >= clk_data->num) { > hw = PTR_ERR(-EINVAL); > goto err; > } > > Thoughts?
That works for me as well. This was more my debug code cleaned up. I assume we still want the warning / error message though?
The other choice would be adding .num_clks for CLK_XXX_NR_CLKS to mtk_clk_desc. What are your thoughts on that?
ChenYu
> Cheers! > Angelo > > > drivers/clk/mediatek/clk-gate.c | 11 +++++++++ > > drivers/clk/mediatek/clk-mtk.c | 43 +++++++++++++++++++++++++++++++++ > > 2 files changed, 54 insertions(+) > > > > diff --git a/drivers/clk/mediatek/clk-gate.c b/drivers/clk/mediatek/clk-gate.c > > index 67d9e741c5e7..bb7c536ef60f 100644 > > --- a/drivers/clk/mediatek/clk-gate.c > > +++ b/drivers/clk/mediatek/clk-gate.c > > @@ -222,6 +222,11 @@ int mtk_clk_register_gates(struct device *dev, struct device_node *node, > > for (i = 0; i < num; i++) { > > const struct mtk_gate *gate = &clks[i]; > > > > + if (WARN(gate->id >= clk_data->num, > > + "%pOF: gateclock ID (%d)larger than expected (%d)\n", > > + node, gate->id, clk_data->num)) > > + continue; > > + > > if (!IS_ERR_OR_NULL(clk_data->hws[gate->id])) { > > pr_warn("%pOF: Trying to register duplicate clock ID: %d\n", > > node, gate->id); > > @@ -251,6 +256,9 @@ int mtk_clk_register_gates(struct device *dev, struct device_node *node, > > while (--i >= 0) { > > const struct mtk_gate *gate = &clks[i]; > > > > + if (gate->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[gate->id])) > > continue; > > > > @@ -273,6 +281,9 @@ void mtk_clk_unregister_gates(const struct mtk_gate *clks, int num, > > for (i = num; i > 0; i--) { > > const struct mtk_gate *gate = &clks[i - 1]; > > > > + if (gate->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[gate->id])) > > continue; > > > > diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c > > index 2e55368dc4d8..09d50a15db77 100644 > > --- a/drivers/clk/mediatek/clk-mtk.c > > +++ b/drivers/clk/mediatek/clk-mtk.c > > @@ -94,6 +94,10 @@ int mtk_clk_register_fixed_clks(const struct mtk_fixed_clk *clks, int num, > > for (i = 0; i < num; i++) { > > const struct mtk_fixed_clk *rc = &clks[i]; > > > > + if (WARN(rc->id >= clk_data->num, > > + "Fixed clock ID (%d) larger than expected (%d)\n", rc->id, clk_data->num)) > > + continue; > > + > > if (!IS_ERR_OR_NULL(clk_data->hws[rc->id])) { > > pr_warn("Trying to register duplicate clock ID: %d\n", rc->id); > > continue; > > @@ -117,6 +121,9 @@ int mtk_clk_register_fixed_clks(const struct mtk_fixed_clk *clks, int num, > > while (--i >= 0) { > > const struct mtk_fixed_clk *rc = &clks[i]; > > > > + if (rc->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[rc->id])) > > continue; > > > > @@ -139,6 +146,9 @@ void mtk_clk_unregister_fixed_clks(const struct mtk_fixed_clk *clks, int num, > > for (i = num; i > 0; i--) { > > const struct mtk_fixed_clk *rc = &clks[i - 1]; > > > > + if (rc->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[rc->id])) > > continue; > > > > @@ -160,6 +170,11 @@ int mtk_clk_register_factors(const struct mtk_fixed_factor *clks, int num, > > for (i = 0; i < num; i++) { > > const struct mtk_fixed_factor *ff = &clks[i]; > > > > + if (WARN(ff->id >= clk_data->num, > > + "Fixed factor clock ID (%d) larger than expected (%d)\n", > > + ff->id, clk_data->num)) > > + continue; > > + > > if (!IS_ERR_OR_NULL(clk_data->hws[ff->id])) { > > pr_warn("Trying to register duplicate clock ID: %d\n", ff->id); > > continue; > > @@ -183,6 +198,9 @@ int mtk_clk_register_factors(const struct mtk_fixed_factor *clks, int num, > > while (--i >= 0) { > > const struct mtk_fixed_factor *ff = &clks[i]; > > > > + if (ff->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[ff->id])) > > continue; > > > > @@ -205,6 +223,9 @@ void mtk_clk_unregister_factors(const struct mtk_fixed_factor *clks, int num, > > for (i = num; i > 0; i--) { > > const struct mtk_fixed_factor *ff = &clks[i - 1]; > > > > + if (ff->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[ff->id])) > > continue; > > > > @@ -339,6 +360,11 @@ int mtk_clk_register_composites(struct device *dev, > > for (i = 0; i < num; i++) { > > const struct mtk_composite *mc = &mcs[i]; > > > > + if (WARN(mc->id >= clk_data->num, > > + "Composite clock ID (%d) larger than expected (%d)\n", > > + mc->id, clk_data->num)) > > + continue; > > + > > if (!IS_ERR_OR_NULL(clk_data->hws[mc->id])) { > > pr_warn("Trying to register duplicate clock ID: %d\n", > > mc->id); > > @@ -362,6 +388,9 @@ int mtk_clk_register_composites(struct device *dev, > > while (--i >= 0) { > > const struct mtk_composite *mc = &mcs[i]; > > > > + if (mc->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[mcs->id])) > > continue; > > > > @@ -384,6 +413,9 @@ void mtk_clk_unregister_composites(const struct mtk_composite *mcs, int num, > > for (i = num; i > 0; i--) { > > const struct mtk_composite *mc = &mcs[i - 1]; > > > > + if (mc->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[mc->id])) > > continue; > > > > @@ -407,6 +439,11 @@ int mtk_clk_register_dividers(struct device *dev, > > for (i = 0; i < num; i++) { > > const struct mtk_clk_divider *mcd = &mcds[i]; > > > > + if (WARN(mcd->id >= clk_data->num, > > + "Divider clock ID (%d) larger than expected (%d)\n", > > + mcd->id, clk_data->num)) > > + continue; > > + > > if (!IS_ERR_OR_NULL(clk_data->hws[mcd->id])) { > > pr_warn("Trying to register duplicate clock ID: %d\n", > > mcd->id); > > @@ -432,6 +469,9 @@ int mtk_clk_register_dividers(struct device *dev, > > while (--i >= 0) { > > const struct mtk_clk_divider *mcd = &mcds[i]; > > > > + if (mcd->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[mcd->id])) > > continue; > > > > @@ -454,6 +494,9 @@ void mtk_clk_unregister_dividers(const struct mtk_clk_divider *mcds, int num, > > for (i = num; i > 0; i--) { > > const struct mtk_clk_divider *mcd = &mcds[i - 1]; > > > > + if (mcd->id >= clk_data->num) > > + continue; > > + > > if (IS_ERR_OR_NULL(clk_data->hws[mcd->id])) > > continue; > > >
| |