Re: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size

From: Chen-Yu Tsai
Date: Wed Jul 19 2023 - 04:53:08 EST

Next message: Lee Jones: "Re: [PATCH] mfd: qcom-pm8xxx: Fix potential deadlock on &chip->pm_irq_lock"
Previous message: Fenglin Wu: "Re: [PATCH v1] led: flash: various minor fixes for leds-qcom-flash driver"
In reply to: AngeloGioacchino Del Regno: "Re: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size"
Next in thread: AngeloGioacchino Del Regno: "Re: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jul 19, 2023 at 4:36 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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;
> >
>

Next message: Lee Jones: "Re: [PATCH] mfd: qcom-pm8xxx: Fix potential deadlock on &chip->pm_irq_lock"
Previous message: Fenglin Wu: "Re: [PATCH v1] led: flash: various minor fixes for leds-qcom-flash driver"
In reply to: AngeloGioacchino Del Regno: "Re: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size"
Next in thread: AngeloGioacchino Del Regno: "Re: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]