lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230719082033.1229277-1-wenst@chromium.org>
Date:   Wed, 19 Jul 2023 16:20:32 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Stephen Boyd <sboyd@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     Chen-Yu Tsai <wenst@...omium.org>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: [PATCH] clk: mediatek: Check if clock ID is larger than clk_hw_onecell_data size

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@...omium.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?

 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;
 
-- 
2.41.0.455.g037347b96a-goog

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ