From: "Gustavo A. R. Silva" <gustavoars@kernel.org> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> Cc: Kees Cook <keescook@chromium.org>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" <gustavoars@kernel.org>, linux-hardening@vger.kernel.org Subject: [PATCH v2 1/2][next] clk: visconti: Fix undefined behavior bug in struct visconti_pll_provider Date: Mon, 16 Oct 2023 16:05:27 -0600 [thread overview] Message-ID: <57a831d94ee2b3889b11525d4ad500356f89576f.1697492890.git.gustavoars@kernel.org> (raw) In-Reply-To: <cover.1697492890.git.gustavoars@kernel.org> `struct clk_hw_onecell_data` is a flexible structure, which means that it contains flexible-array member at the bottom, in this case array `hws`: include/linux/clk-provider.h: 1380 struct clk_hw_onecell_data { 1381 unsigned int num; 1382 struct clk_hw *hws[] __counted_by(num); 1383 }; This could potentially lead to an overwrite of the objects following `clk_data` in `struct visconti_pll_provider`, in this case `struct device_node *node;`, at run-time: drivers/clk/visconti/pll.h: 16 struct visconti_pll_provider { 17 void __iomem *reg_base; 18 struct clk_hw_onecell_data clk_data; 19 struct device_node *node; 20 }; Notice that a total of 56 bytes are allocated for flexible-array `hws` at line 328. See below: include/dt-bindings/clock/toshiba,tmpv770x.h: 14 #define TMPV770X_NR_PLL 7 drivers/clk/visconti/pll-tmpv770x.c: 69 ctx = visconti_init_pll(np, reg_base, TMPV770X_NR_PLL); drivers/clk/visconti/pll.c: 321 struct visconti_pll_provider * __init visconti_init_pll(struct device_node *np, 322 void __iomem *base, 323 unsigned long nr_plls) 324 { 325 struct visconti_pll_provider *ctx; ... 328 ctx = kzalloc(struct_size(ctx, clk_data.hws, nr_plls), GFP_KERNEL); `struct_size(ctx, clk_data.hws, nr_plls)` above translates to sizeof(struct visconti_pll_provider) + sizeof(struct clk_hw *) * 7 == 24 + 8 * 7 == 24 + 56 ^^^^ | allocated bytes for flex array `hws` $ pahole -C visconti_pll_provider drivers/clk/visconti/pll.o struct visconti_pll_provider { void * reg_base; /* 0 8 */ struct clk_hw_onecell_data clk_data; /* 8 8 */ struct device_node * node; /* 16 8 */ /* size: 24, cachelines: 1, members: 3 */ /* last cacheline: 24 bytes */ }; And then, after the allocation, some data is written into all members of `struct visconti_pll_provider`: 332 for (i = 0; i < nr_plls; ++i) 333 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT); 334 335 ctx->node = np; 336 ctx->reg_base = base; 337 ctx->clk_data.num = nr_plls; Fix all these by placing the declaration of object `clk_data` at the end of `struct visconti_pll_provider`. Also, add a comment to make it clear that this object must always be last in the structure, and prevent this bug from being introduced again in the future. -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally. Fixes: b4cbe606dc36 ("clk: visconti: Add support common clock driver and reset driver") Cc: stable@vger.kernel.org Reviewed-by: Kees Cook <keescook@chromium.org> Acked-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - Mention -Wflex-array-member-not-at-end in the changelog text. v1: - Link: https://lore.kernel.org/linux-hardening/0a59a721d54b557076cc94eabeb694d463773204.1697076650.git.gustavoars@kernel.org/ drivers/clk/visconti/pll.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/clk/visconti/pll.h b/drivers/clk/visconti/pll.h index 01d07f1bf01b..c4bd40676da4 100644 --- a/drivers/clk/visconti/pll.h +++ b/drivers/clk/visconti/pll.h @@ -15,8 +15,10 @@ struct visconti_pll_provider { void __iomem *reg_base; - struct clk_hw_onecell_data clk_data; struct device_node *node; + + /* Must be last */ + struct clk_hw_onecell_data clk_data; }; #define VISCONTI_PLL_RATE(_rate, _dacen, _dsmen, \ -- 2.34.1
WARNING: multiple messages have this Message-ID (diff)
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> Cc: Kees Cook <keescook@chromium.org>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" <gustavoars@kernel.org>, linux-hardening@vger.kernel.org Subject: [PATCH v2 1/2][next] clk: visconti: Fix undefined behavior bug in struct visconti_pll_provider Date: Mon, 16 Oct 2023 16:05:27 -0600 [thread overview] Message-ID: <57a831d94ee2b3889b11525d4ad500356f89576f.1697492890.git.gustavoars@kernel.org> (raw) In-Reply-To: <cover.1697492890.git.gustavoars@kernel.org> `struct clk_hw_onecell_data` is a flexible structure, which means that it contains flexible-array member at the bottom, in this case array `hws`: include/linux/clk-provider.h: 1380 struct clk_hw_onecell_data { 1381 unsigned int num; 1382 struct clk_hw *hws[] __counted_by(num); 1383 }; This could potentially lead to an overwrite of the objects following `clk_data` in `struct visconti_pll_provider`, in this case `struct device_node *node;`, at run-time: drivers/clk/visconti/pll.h: 16 struct visconti_pll_provider { 17 void __iomem *reg_base; 18 struct clk_hw_onecell_data clk_data; 19 struct device_node *node; 20 }; Notice that a total of 56 bytes are allocated for flexible-array `hws` at line 328. See below: include/dt-bindings/clock/toshiba,tmpv770x.h: 14 #define TMPV770X_NR_PLL 7 drivers/clk/visconti/pll-tmpv770x.c: 69 ctx = visconti_init_pll(np, reg_base, TMPV770X_NR_PLL); drivers/clk/visconti/pll.c: 321 struct visconti_pll_provider * __init visconti_init_pll(struct device_node *np, 322 void __iomem *base, 323 unsigned long nr_plls) 324 { 325 struct visconti_pll_provider *ctx; ... 328 ctx = kzalloc(struct_size(ctx, clk_data.hws, nr_plls), GFP_KERNEL); `struct_size(ctx, clk_data.hws, nr_plls)` above translates to sizeof(struct visconti_pll_provider) + sizeof(struct clk_hw *) * 7 == 24 + 8 * 7 == 24 + 56 ^^^^ | allocated bytes for flex array `hws` $ pahole -C visconti_pll_provider drivers/clk/visconti/pll.o struct visconti_pll_provider { void * reg_base; /* 0 8 */ struct clk_hw_onecell_data clk_data; /* 8 8 */ struct device_node * node; /* 16 8 */ /* size: 24, cachelines: 1, members: 3 */ /* last cacheline: 24 bytes */ }; And then, after the allocation, some data is written into all members of `struct visconti_pll_provider`: 332 for (i = 0; i < nr_plls; ++i) 333 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT); 334 335 ctx->node = np; 336 ctx->reg_base = base; 337 ctx->clk_data.num = nr_plls; Fix all these by placing the declaration of object `clk_data` at the end of `struct visconti_pll_provider`. Also, add a comment to make it clear that this object must always be last in the structure, and prevent this bug from being introduced again in the future. -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally. Fixes: b4cbe606dc36 ("clk: visconti: Add support common clock driver and reset driver") Cc: stable@vger.kernel.org Reviewed-by: Kees Cook <keescook@chromium.org> Acked-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - Mention -Wflex-array-member-not-at-end in the changelog text. v1: - Link: https://lore.kernel.org/linux-hardening/0a59a721d54b557076cc94eabeb694d463773204.1697076650.git.gustavoars@kernel.org/ drivers/clk/visconti/pll.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/clk/visconti/pll.h b/drivers/clk/visconti/pll.h index 01d07f1bf01b..c4bd40676da4 100644 --- a/drivers/clk/visconti/pll.h +++ b/drivers/clk/visconti/pll.h @@ -15,8 +15,10 @@ struct visconti_pll_provider { void __iomem *reg_base; - struct clk_hw_onecell_data clk_data; struct device_node *node; + + /* Must be last */ + struct clk_hw_onecell_data clk_data; }; #define VISCONTI_PLL_RATE(_rate, _dacen, _dsmen, \ -- 2.34.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-10-16 22:05 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-16 22:04 [PATCH v2 0/2][next] clk: visconti: Fix undefined behavior bug and add bounds-checking coverage Gustavo A. R. Silva 2023-10-16 22:04 ` Gustavo A. R. Silva 2023-10-16 22:05 ` Gustavo A. R. Silva [this message] 2023-10-16 22:05 ` [PATCH v2 1/2][next] clk: visconti: Fix undefined behavior bug in struct visconti_pll_provider Gustavo A. R. Silva 2023-10-24 2:40 ` Stephen Boyd 2023-10-24 2:40 ` Stephen Boyd 2023-10-24 2:48 ` Gustavo A. R. Silva 2023-10-24 2:48 ` Gustavo A. R. Silva 2023-10-24 2:52 ` Stephen Boyd 2023-10-24 2:52 ` Stephen Boyd 2023-10-16 22:06 ` [PATCH v2 2/2][next] clk: visconti: Add bounds-checking coverage for " Gustavo A. R. Silva 2023-10-16 22:06 ` Gustavo A. R. Silva 2023-10-24 2:40 ` Stephen Boyd 2023-10-24 2:40 ` Stephen Boyd
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=57a831d94ee2b3889b11525d4ad500356f89576f.1697492890.git.gustavoars@kernel.org \ --to=gustavoars@kernel.org \ --cc=keescook@chromium.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-hardening@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mturquette@baylibre.com \ --cc=nobuhiro1.iwamatsu@toshiba.co.jp \ --cc=sboyd@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.