linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4
@ 2020-02-20 16:29 Leonard Crestez
  2020-02-20 16:29 ` [PATCH v2 1/8] clk: imx: Align imx sc clock " Leonard Crestez
                   ` (7 more replies)
  0 siblings, 8 replies; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have sizeof "6" and "7".

This produces many oopses with CONFIG_KASAN=y:

	BUG: KASAN: stack-out-of-bounds in imx_mu_send_data+0x108/0x1f0

It shouldn't cause an issues in normal use because these structs are
always allocated on the stack but tools like KASAN are very useful on
stable kernels.

Chnages since v1:
* Split into many patches with individual fixes: tags
Link to v1: https://patchwork.kernel.org/patch/11376909/

Leonard Crestez (8):
  clk: imx: Align imx sc clock msg structs to 4
  clk: imx: Align imx sc clock parent msg structs to 4
  firmware: imx: misc: Align imx sc msg structs to 4
  firmware: imx: scu-pd: Align imx sc msg structs to 4
  firmware: imx: Align imx_sc_msg_req_cpu_start to 4
  pinctrl: imx: scu: Align imx sc msg structs to 4
  rtc: imx-sc: Align imx sc msg structs to 4
  soc: imx-scu: Align imx sc msg structs to 4

 drivers/clk/imx/clk-scu.c               | 8 ++++----
 drivers/firmware/imx/misc.c             | 8 ++++----
 drivers/firmware/imx/scu-pd.c           | 2 +-
 drivers/pinctrl/freescale/pinctrl-scu.c | 4 ++--
 drivers/rtc/rtc-imx-sc.c                | 2 +-
 drivers/soc/imx/soc-imx-scu.c           | 2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-24  7:22   ` Shawn Guo
  2020-02-25 16:51   ` Stephen Boyd
  2020-02-20 16:29 ` [PATCH v2 2/8] clk: imx: Align imx sc clock parent " Leonard Crestez
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/clk/imx/clk-scu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index fbef740704d0..3c5c42d8833e 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -41,16 +41,16 @@ struct clk_scu {
 struct imx_sc_msg_req_set_clock_rate {
 	struct imx_sc_rpc_msg hdr;
 	__le32 rate;
 	__le16 resource;
 	u8 clk;
-} __packed;
+} __packed __aligned(4);
 
 struct req_get_clock_rate {
 	__le16 resource;
 	u8 clk;
-} __packed;
+} __packed __aligned(4);
 
 struct resp_get_clock_rate {
 	__le32 rate;
 };
 
@@ -119,11 +119,11 @@ struct imx_sc_msg_req_clock_enable {
 	struct imx_sc_rpc_msg hdr;
 	__le16 resource;
 	u8 clk;
 	u8 enable;
 	u8 autog;
-} __packed;
+} __packed __aligned(4);
 
 static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
 {
 	return container_of(hw, struct clk_scu, hw);
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 2/8] clk: imx: Align imx sc clock parent msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
  2020-02-20 16:29 ` [PATCH v2 1/8] clk: imx: Align imx sc clock " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-24  7:23   ` Shawn Guo
  2020-02-20 16:29 ` [PATCH v2 3/8] firmware: imx: misc: Align imx sc " Leonard Crestez
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: 666aed2d13ee ("clk: imx: scu: add set parent support")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/clk/imx/clk-scu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index 3c5c42d8833e..b8b2072742a5 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -82,11 +82,11 @@ struct imx_sc_msg_get_clock_parent {
 	struct imx_sc_rpc_msg hdr;
 	union {
 		struct req_get_clock_parent {
 			__le16 resource;
 			u8 clk;
-		} __packed req;
+		} __packed __aligned(4) req;
 		struct resp_get_clock_parent {
 			u8 parent;
 		} resp;
 	} data;
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 3/8] firmware: imx: misc: Align imx sc msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
  2020-02-20 16:29 ` [PATCH v2 1/8] clk: imx: Align imx sc clock " Leonard Crestez
  2020-02-20 16:29 ` [PATCH v2 2/8] clk: imx: Align imx sc clock parent " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-24  7:28   ` Shawn Guo
  2020-02-20 16:29 ` [PATCH v2 4/8] firmware: imx: scu-pd: " Leonard Crestez
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y:

    BUG: KASAN: stack-out-of-bounds in imx_mu_send_data+0x108/0x1f0

It shouldn't cause an issues in normal use because these structs are
always allocated on the stack.

Fixes: 15e1f2bc8b3b ("firmware: imx: add misc svc support")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/firmware/imx/misc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
index 4b56a587dacd..6a21ff942f82 100644
--- a/drivers/firmware/imx/misc.c
+++ b/drivers/firmware/imx/misc.c
@@ -14,11 +14,11 @@
 struct imx_sc_msg_req_misc_set_ctrl {
 	struct imx_sc_rpc_msg hdr;
 	u32 ctrl;
 	u32 val;
 	u16 resource;
-} __packed;
+} __packed __aligned(4);
 
 struct imx_sc_msg_req_cpu_start {
 	struct imx_sc_rpc_msg hdr;
 	u32 address_hi;
 	u32 address_lo;
@@ -28,16 +28,16 @@ struct imx_sc_msg_req_cpu_start {
 
 struct imx_sc_msg_req_misc_get_ctrl {
 	struct imx_sc_rpc_msg hdr;
 	u32 ctrl;
 	u16 resource;
-} __packed;
+} __packed __aligned(4);
 
 struct imx_sc_msg_resp_misc_get_ctrl {
 	struct imx_sc_rpc_msg hdr;
 	u32 val;
-} __packed;
+} __packed __aligned(4);
 
 /*
  * This function sets a miscellaneous control value.
  *
  * @param[in]     ipc         IPC handle
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 4/8] firmware: imx: scu-pd: Align imx sc msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
                   ` (2 preceding siblings ...)
  2020-02-20 16:29 ` [PATCH v2 3/8] firmware: imx: misc: Align imx sc " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-24  7:28   ` Shawn Guo
  2020-02-20 16:29 ` [PATCH v2 5/8] firmware: imx: Align imx_sc_msg_req_cpu_start " Leonard Crestez
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: c800cd7824bd ("firmware: imx: add SCU power domain driver")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/firmware/imx/scu-pd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
index 09cfa268c6bd..cbc9a47ff44f 100644
--- a/drivers/firmware/imx/scu-pd.c
+++ b/drivers/firmware/imx/scu-pd.c
@@ -59,11 +59,11 @@
 /* SCU Power Mode Protocol definition */
 struct imx_sc_msg_req_set_resource_power_mode {
 	struct imx_sc_rpc_msg hdr;
 	u16 resource;
 	u8 mode;
-} __packed;
+} __packed __aligned(4);
 
 #define IMX_SCU_PD_NAME_SIZE 20
 struct imx_sc_pm_domain {
 	struct generic_pm_domain pd;
 	char name[IMX_SCU_PD_NAME_SIZE];
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 5/8] firmware: imx: Align imx_sc_msg_req_cpu_start to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
                   ` (3 preceding siblings ...)
  2020-02-20 16:29 ` [PATCH v2 4/8] firmware: imx: scu-pd: " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-24  7:28   ` Shawn Guo
  2020-02-20 16:29 ` [PATCH v2 6/8] pinctrl: imx: scu: Align imx sc msg structs " Leonard Crestez
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: d90bf296ae18 ("firmware: imx: Add support to start/stop a CPU")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/firmware/imx/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
index 6a21ff942f82..d073cb3ce699 100644
--- a/drivers/firmware/imx/misc.c
+++ b/drivers/firmware/imx/misc.c
@@ -22,11 +22,11 @@ struct imx_sc_msg_req_cpu_start {
 	struct imx_sc_rpc_msg hdr;
 	u32 address_hi;
 	u32 address_lo;
 	u16 resource;
 	u8 enable;
-} __packed;
+} __packed __aligned(4);
 
 struct imx_sc_msg_req_misc_get_ctrl {
 	struct imx_sc_rpc_msg hdr;
 	u32 ctrl;
 	u16 resource;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 6/8] pinctrl: imx: scu: Align imx sc msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
                   ` (4 preceding siblings ...)
  2020-02-20 16:29 ` [PATCH v2 5/8] firmware: imx: Align imx_sc_msg_req_cpu_start " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-21 15:34   ` Linus Walleij
  2020-02-20 16:29 ` [PATCH v2 7/8] rtc: imx-sc: " Leonard Crestez
  2020-02-20 16:29 ` [PATCH v2 8/8] soc: imx-scu: " Leonard Crestez
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: b96eea718bf6 ("pinctrl: fsl: add scu based pinctrl support")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-scu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-scu.c b/drivers/pinctrl/freescale/pinctrl-scu.c
index 73bf1d9f9cc6..23cf04bdfc55 100644
--- a/drivers/pinctrl/freescale/pinctrl-scu.c
+++ b/drivers/pinctrl/freescale/pinctrl-scu.c
@@ -21,16 +21,16 @@ enum pad_func_e {
 
 struct imx_sc_msg_req_pad_set {
 	struct imx_sc_rpc_msg hdr;
 	u32 val;
 	u16 pad;
-} __packed;
+} __packed __aligned(4);
 
 struct imx_sc_msg_req_pad_get {
 	struct imx_sc_rpc_msg hdr;
 	u16 pad;
-} __packed;
+} __packed __aligned(4);
 
 struct imx_sc_msg_resp_pad_get {
 	struct imx_sc_rpc_msg hdr;
 	u32 val;
 } __packed;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 7/8] rtc: imx-sc: Align imx sc msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
                   ` (5 preceding siblings ...)
  2020-02-20 16:29 ` [PATCH v2 6/8] pinctrl: imx: scu: Align imx sc msg structs " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-03-03 11:02   ` Alexandre Belloni
  2020-02-20 16:29 ` [PATCH v2 8/8] soc: imx-scu: " Leonard Crestez
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: a3094fc1a15e ("rtc: imx-sc: add rtc alarm support")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-imx-sc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-imx-sc.c b/drivers/rtc/rtc-imx-sc.c
index cf2c12107f2b..a5f59e6f862e 100644
--- a/drivers/rtc/rtc-imx-sc.c
+++ b/drivers/rtc/rtc-imx-sc.c
@@ -35,11 +35,11 @@ struct imx_sc_msg_timer_rtc_set_alarm {
 	u8 mon;
 	u8 day;
 	u8 hour;
 	u8 min;
 	u8 sec;
-} __packed;
+} __packed __aligned(4);
 
 static int imx_sc_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct imx_sc_msg_timer_get_rtc_time msg;
 	struct imx_sc_rpc_msg *hdr = &msg.hdr;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 8/8] soc: imx-scu: Align imx sc msg structs to 4
  2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
                   ` (6 preceding siblings ...)
  2020-02-20 16:29 ` [PATCH v2 7/8] rtc: imx-sc: " Leonard Crestez
@ 2020-02-20 16:29 ` Leonard Crestez
  2020-02-24  7:30   ` Shawn Guo
  7 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-20 16:29 UTC (permalink / raw)
  To: Shawn Guo, Dong Aisheng
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: 73feb4d0f8f1 ("soc: imx-scu: Add SoC UID(unique identifier) support")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/soc/imx/soc-imx-scu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
index fb70b8a3f7c5..20d37eaeb5f2 100644
--- a/drivers/soc/imx/soc-imx-scu.c
+++ b/drivers/soc/imx/soc-imx-scu.c
@@ -23,11 +23,11 @@ struct imx_sc_msg_misc_get_soc_id {
 		} __packed req;
 		struct {
 			u32 id;
 		} resp;
 	} data;
-} __packed;
+} __packed __aligned(4);
 
 struct imx_sc_msg_misc_get_soc_uid {
 	struct imx_sc_rpc_msg hdr;
 	u32 uid_low;
 	u32 uid_high;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 6/8] pinctrl: imx: scu: Align imx sc msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 6/8] pinctrl: imx: scu: Align imx sc msg structs " Leonard Crestez
@ 2020-02-21 15:34   ` Linus Walleij
  0 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2020-02-21 15:34 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	Franck LENORMAND, open list:GPIO SUBSYSTEM, NXP Linux Team,
	Sascha Hauer, Fabio Estevam, Shawn Guo, linux-clk, Linux ARM,
	linux-rtc

On Thu, Feb 20, 2020 at 5:29 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:

> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
>
> This produces many oopses with CONFIG_KASAN=y.
>
> Fix by marking with __aligned(4).
>
> Fixes: b96eea718bf6 ("pinctrl: fsl: add scu based pinctrl support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Patch applied for fixes. KASan needs to work.
Thanks for fixing this!

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 1/8] clk: imx: Align imx sc clock " Leonard Crestez
@ 2020-02-24  7:22   ` Shawn Guo
  2020-03-16  0:25     ` Shawn Guo
  2020-02-25 16:51   ` Stephen Boyd
  1 sibling, 1 reply; 23+ messages in thread
From: Shawn Guo @ 2020-02-24  7:22 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On Thu, Feb 20, 2020 at 06:29:32PM +0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 2/8] clk: imx: Align imx sc clock parent msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 2/8] clk: imx: Align imx sc clock parent " Leonard Crestez
@ 2020-02-24  7:23   ` Shawn Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Shawn Guo @ 2020-02-24  7:23 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On Thu, Feb 20, 2020 at 06:29:33PM +0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: 666aed2d13ee ("clk: imx: scu: add set parent support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/8] firmware: imx: misc: Align imx sc msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 3/8] firmware: imx: misc: Align imx sc " Leonard Crestez
@ 2020-02-24  7:28   ` Shawn Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Shawn Guo @ 2020-02-24  7:28 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On Thu, Feb 20, 2020 at 06:29:34PM +0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y:
> 
>     BUG: KASAN: stack-out-of-bounds in imx_mu_send_data+0x108/0x1f0
> 
> It shouldn't cause an issues in normal use because these structs are
> always allocated on the stack.
> 
> Fixes: 15e1f2bc8b3b ("firmware: imx: add misc svc support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 4/8] firmware: imx: scu-pd: Align imx sc msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 4/8] firmware: imx: scu-pd: " Leonard Crestez
@ 2020-02-24  7:28   ` Shawn Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Shawn Guo @ 2020-02-24  7:28 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On Thu, Feb 20, 2020 at 06:29:35PM +0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: c800cd7824bd ("firmware: imx: add SCU power domain driver")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 5/8] firmware: imx: Align imx_sc_msg_req_cpu_start to 4
  2020-02-20 16:29 ` [PATCH v2 5/8] firmware: imx: Align imx_sc_msg_req_cpu_start " Leonard Crestez
@ 2020-02-24  7:28   ` Shawn Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Shawn Guo @ 2020-02-24  7:28 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On Thu, Feb 20, 2020 at 06:29:36PM +0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: d90bf296ae18 ("firmware: imx: Add support to start/stop a CPU")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 8/8] soc: imx-scu: Align imx sc msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 8/8] soc: imx-scu: " Leonard Crestez
@ 2020-02-24  7:30   ` Shawn Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Shawn Guo @ 2020-02-24  7:30 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On Thu, Feb 20, 2020 at 06:29:39PM +0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: 73feb4d0f8f1 ("soc: imx-scu: Add SoC UID(unique identifier) support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 1/8] clk: imx: Align imx sc clock " Leonard Crestez
  2020-02-24  7:22   ` Shawn Guo
@ 2020-02-25 16:51   ` Stephen Boyd
  2020-02-25 19:52     ` Leonard Crestez
  2020-03-16  8:52     ` Aisheng Dong
  1 sibling, 2 replies; 23+ messages in thread
From: Stephen Boyd @ 2020-02-25 16:51 UTC (permalink / raw)
  To: Dong Aisheng, Leonard Crestez, Shawn Guo
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Michael Turquette, Stefan Agner, linux-clk,
	Franck LENORMAND, linux-gpio, linux-imx, kernel, Fabio Estevam,
	Linus Walleij, linux-arm-kernel

Quoting Leonard Crestez (2020-02-20 08:29:32)
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/clk/imx/clk-scu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> index fbef740704d0..3c5c42d8833e 100644
> --- a/drivers/clk/imx/clk-scu.c
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -41,16 +41,16 @@ struct clk_scu {
>  struct imx_sc_msg_req_set_clock_rate {
>         struct imx_sc_rpc_msg hdr;
>         __le32 rate;
>         __le16 resource;
>         u8 clk;
> -} __packed;
> +} __packed __aligned(4);

Sorry, this still doesn't make sense to me. Having __aligned(4) means
that the struct is placed on the stack at some alignment, great, but it
still has __packed so the sizeof this struct is some odd number like 11.
If this struct is the last element on the stack it will end at some
unaligned address and the mailbox code will read a few bytes beyond the
end of the stack.

I see that the calling code puts 3 as the 'size' for this struct in
clk_scu_set_rate().

	hdr->size = 3;

That seems to say that the struct is 3 words long, or 12 bytes. Then we
call imx_scu_call_rpc(), passing the pointer to this struct on the stack
and that eventually gets into imx_scu_ipc_write() calling
mbox_send_message() with u32 pointers.

	for (i = 0; i < hdr->size; i++) {
		sc_chan = &sc_ipc->chans[i % 4];
		ret = mbox_send_message(sc_chan->ch, &data[i]);

So we've taken the 11 byte struct (data in this case) and casted it to a
u32 array with 3 elements, which is bad. This is what kasan is warning
about. Adding aligned sometimes fixes it because the compiler will place
the next stack variable at the naturally aligned location and thus we
get the one byte padding but I don't see how that works when it's the
last stack element. The stack will end at some unaligned address.

The better solution would be to drop __aligned(4) and make a union of
the struct with whatever size number of words the message is or do a
copy of the struct into a u32 array that is passed to
imx_scu_call_rpc().

For example:

	struct imx_sc_msg_req_set_clock_rate {
		union {
			struct packed_message {
				struct imx_sc_rpc_msg hdr;
				__le32 rate;
				__le16 resource;
				u8 clk;
			} __packed;
			u32 data[3];
		};
	};

If the union approach was used then each time imx_scu_call_rpc() is
called we can simply pass the 'data' member and make the second argument
'msg' strongly typed to be a u32 pointer. kasan should be happy too.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-02-25 16:51   ` Stephen Boyd
@ 2020-02-25 19:52     ` Leonard Crestez
       [not found]       ` <158276809953.177367.6095692240077023796@swboyd.mtv.corp.google.com>
  2020-03-16  8:52     ` Aisheng Dong
  1 sibling, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-02-25 19:52 UTC (permalink / raw)
  To: Stephen Boyd, Shawn Guo
  Cc: Aisheng Dong, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Michael Turquette, Stefan Agner, linux-clk,
	Franck Lenormand, linux-gpio, dl-linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel, linux-rtc

On 25.02.2020 18:52, Stephen Boyd wrote:
> Quoting Leonard Crestez (2020-02-20 08:29:32)
>> The imx SC api strongly assumes that messages are composed out of
>> 4-bytes words but some of our message structs have odd sizeofs.
>>
>> This produces many oopses with CONFIG_KASAN=y.
>>
>> Fix by marking with __aligned(4).
>>
>> Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/clk/imx/clk-scu.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
>> index fbef740704d0..3c5c42d8833e 100644
>> --- a/drivers/clk/imx/clk-scu.c
>> +++ b/drivers/clk/imx/clk-scu.c
>> @@ -41,16 +41,16 @@ struct clk_scu {
>>   struct imx_sc_msg_req_set_clock_rate {
>>          struct imx_sc_rpc_msg hdr;
>>          __le32 rate;
>>          __le16 resource;
>>          u8 clk;
>> -} __packed;
>> +} __packed __aligned(4);
> 
> Sorry, this still doesn't make sense to me. Having __aligned(4) means
> that the struct is placed on the stack at some alignment, great, but it
> still has __packed so the sizeof this struct is some odd number like 11.
> If this struct is the last element on the stack it will end at some
> unaligned address and the mailbox code will read a few bytes beyond the
> end of the stack.

I checked again and marking the struct with __aligned(4) makes it have 
sizeof == 12 as intended. It was 11 before.

     static_assert(sizeof(struct imx_sc_msg_req_set_clock_rate) == 12);

After reading through your email and gcc docs again I'm not sure if this 
portable/reliable this is but as far as I understand "sizeof" needs to 
account for alignment. Or is this just an accident with my compiler?

Marking a structure both __packed and __aligned(4) means that __packed 
only affects internal struct member layout but sizeof is still rounded 
up to a multiple of 4:

struct test {
	u8	a;
	u16	b;
} __packed __aligned(4);

static_assert(sizeof(struct test) == 4);
static_assert(offsetof(struct test, a) == 0);
static_assert(offsetof(struct test, b) == 1);

This test is not realistic because I don't think SCU messages have any 
such oddly-aligned members.

> 
> I see that the calling code puts 3 as the 'size' for this struct in
> clk_scu_set_rate().
> 
> 	hdr->size = 3;
> 
> That seems to say that the struct is 3 words long, or 12 bytes. Then we
> call imx_scu_call_rpc(), passing the pointer to this struct on the stack
> and that eventually gets into imx_scu_ipc_write() calling
> mbox_send_message() with u32 pointers.
> 
> 	for (i = 0; i < hdr->size; i++) {
> 		sc_chan = &sc_ipc->chans[i % 4];
> 		ret = mbox_send_message(sc_chan->ch, &data[i]);
> 
> So we've taken the 11 byte struct (data in this case) and casted it to a
> u32 array with 3 elements, which is bad. This is what kasan is warning
> about. Adding aligned sometimes fixes it because the compiler will place
> the next stack variable at the naturally aligned location and thus we
> get the one byte padding but I don't see how that works when it's the
> last stack element. The stack will end at some unaligned address.
> 
> The better solution would be to drop __aligned(4) and make a union of
> the struct with whatever size number of words the message is or do a
> copy of the struct into a u32 array that is passed to
> imx_scu_call_rpc().
> 
> For example:
> 
> 	struct imx_sc_msg_req_set_clock_rate {
> 		union {
> 			struct packed_message {
> 				struct imx_sc_rpc_msg hdr;
> 				__le32 rate;
> 				__le16 resource;
> 				u8 clk;
> 			} __packed;
> 			u32 data[3];
> 		};
> 	};
> 
> If the union approach was used then each time imx_scu_call_rpc() is
> called we can simply pass the 'data' member and make the second argument
> 'msg' strongly typed to be a u32 pointer. kasan should be happy too.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 7/8] rtc: imx-sc: Align imx sc msg structs to 4
  2020-02-20 16:29 ` [PATCH v2 7/8] rtc: imx-sc: " Leonard Crestez
@ 2020-03-03 11:02   ` Alexandre Belloni
  0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Belloni @ 2020-03-03 11:02 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Abel Vesa, Anson Huang,
	Stephen Boyd, Michael Turquette, Stefan Agner, linux-clk,
	Franck LENORMAND, linux-gpio, linux-imx, kernel, Fabio Estevam,
	Shawn Guo, Linus Walleij, linux-arm-kernel, linux-rtc

On 20/02/2020 18:29:38+0200, Leonard Crestez wrote:
> The imx SC api strongly assumes that messages are composed out of
> 4-bytes words but some of our message structs have odd sizeofs.
> 
> This produces many oopses with CONFIG_KASAN=y.
> 
> Fix by marking with __aligned(4).
> 
> Fixes: a3094fc1a15e ("rtc: imx-sc: add rtc alarm support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-imx-sc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-02-24  7:22   ` Shawn Guo
@ 2020-03-16  0:25     ` Shawn Guo
  0 siblings, 0 replies; 23+ messages in thread
From: Shawn Guo @ 2020-03-16  0:25 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, linux-rtc,
	Stefan Agner, Franck LENORMAND, linux-gpio, linux-imx, kernel,
	Fabio Estevam, linux-clk, Linus Walleij, linux-arm-kernel

On Mon, Feb 24, 2020 at 03:22:18PM +0800, Shawn Guo wrote:
> On Thu, Feb 20, 2020 at 06:29:32PM +0200, Leonard Crestez wrote:
> > The imx SC api strongly assumes that messages are composed out of
> > 4-bytes words but some of our message structs have odd sizeofs.
> > 
> > This produces many oopses with CONFIG_KASAN=y.
> > 
> > Fix by marking with __aligned(4).
> > 
> > Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> Applied, thanks.

Patch #1 and #2 were dropped from my clk queue, as Stephen hasn't been
convinced by this change.

Shawn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-02-25 16:51   ` Stephen Boyd
  2020-02-25 19:52     ` Leonard Crestez
@ 2020-03-16  8:52     ` Aisheng Dong
  1 sibling, 0 replies; 23+ messages in thread
From: Aisheng Dong @ 2020-03-16  8:52 UTC (permalink / raw)
  To: Stephen Boyd, Leonard Crestez, Shawn Guo
  Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Michael Turquette, Stefan Agner, linux-clk,
	Franck Lenormand, linux-gpio, dl-linux-imx, kernel,
	Fabio Estevam, Linus Walleij, linux-arm-kernel

> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Wednesday, February 26, 2020 12:52 AM 
> Quoting Leonard Crestez (2020-02-20 08:29:32)
> > The imx SC api strongly assumes that messages are composed out of
> > 4-bytes words but some of our message structs have odd sizeofs.
> >
> > This produces many oopses with CONFIG_KASAN=y.
> >
> > Fix by marking with __aligned(4).
> >
> > Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> >  drivers/clk/imx/clk-scu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> > index fbef740704d0..3c5c42d8833e 100644
> > --- a/drivers/clk/imx/clk-scu.c
> > +++ b/drivers/clk/imx/clk-scu.c
> > @@ -41,16 +41,16 @@ struct clk_scu {
> >  struct imx_sc_msg_req_set_clock_rate {
> >         struct imx_sc_rpc_msg hdr;
> >         __le32 rate;
> >         __le16 resource;
> >         u8 clk;
> > -} __packed;
> > +} __packed __aligned(4);
> 
> Sorry, this still doesn't make sense to me. Having __aligned(4) means that the
> struct is placed on the stack at some alignment, great, but it still has __packed so
> the sizeof this struct is some odd number like 11

> If this struct is the last element on the stack it will end at some unaligned address
> and the mailbox code will read a few bytes beyond the end of the stack.

Hi Leonard,

Can you construct this case to see if we can reproduce the issue as pointed by Stephen?

Regards
Aisheng

> 
> I see that the calling code puts 3 as the 'size' for this struct in clk_scu_set_rate().
> 
> 	hdr->size = 3;
> 
> That seems to say that the struct is 3 words long, or 12 bytes. Then we call
> imx_scu_call_rpc(), passing the pointer to this struct on the stack and that
> eventually gets into imx_scu_ipc_write() calling
> mbox_send_message() with u32 pointers.
> 
> 	for (i = 0; i < hdr->size; i++) {
> 		sc_chan = &sc_ipc->chans[i % 4];
> 		ret = mbox_send_message(sc_chan->ch, &data[i]);
> 
> So we've taken the 11 byte struct (data in this case) and casted it to a
> u32 array with 3 elements, which is bad. This is what kasan is warning about.
> Adding aligned sometimes fixes it because the compiler will place the next stack
> variable at the naturally aligned location and thus we get the one byte padding
> but I don't see how that works when it's the last stack element. The stack will
> end at some unaligned address.
> 
> The better solution would be to drop __aligned(4) and make a union of the
> struct with whatever size number of words the message is or do a copy of the
> struct into a u32 array that is passed to imx_scu_call_rpc().
> 
> For example:
> 
> 	struct imx_sc_msg_req_set_clock_rate {
> 		union {
> 			struct packed_message {
> 				struct imx_sc_rpc_msg hdr;
> 				__le32 rate;
> 				__le16 resource;
> 				u8 clk;
> 			} __packed;
> 			u32 data[3];
> 		};
> 	};
> 
> If the union approach was used then each time imx_scu_call_rpc() is called we
> can simply pass the 'data' member and make the second argument 'msg'
> strongly typed to be a u32 pointer. kasan should be happy too.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
       [not found]       ` <158276809953.177367.6095692240077023796@swboyd.mtv.corp.google.com>
@ 2020-03-17 19:25         ` Leonard Crestez
  2020-03-17 19:54           ` Dmitry Vyukov
  0 siblings, 1 reply; 23+ messages in thread
From: Leonard Crestez @ 2020-03-17 19:25 UTC (permalink / raw)
  To: Stephen Boyd, Shawn Guo
  Cc: Aisheng Dong, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Michael Turquette, Stefan Agner, linux-clk,
	Franck Lenormand, Alexander Potapenko, dl-linux-imx,
	Fabio Estevam, Andrey Ryabinin, Dmitry Vyukov, kasan-dev,
	Linus Walleij, linux-arm-kernel

On 2020-02-27 3:48 AM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2020-02-25 11:52:11)
>> On 25.02.2020 18:52, Stephen Boyd wrote:
>>> Quoting Leonard Crestez (2020-02-20 08:29:32)
>>>> The imx SC api strongly assumes that messages are composed out of
>>>> 4-bytes words but some of our message structs have odd sizeofs.
>>>>
>>>> This produces many oopses with CONFIG_KASAN=y.
>>>>
>>>> Fix by marking with __aligned(4).
>>>>
>>>> Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    drivers/clk/imx/clk-scu.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
>>>> index fbef740704d0..3c5c42d8833e 100644
>>>> --- a/drivers/clk/imx/clk-scu.c
>>>> +++ b/drivers/clk/imx/clk-scu.c
>>>> @@ -41,16 +41,16 @@ struct clk_scu {
>>>>    struct imx_sc_msg_req_set_clock_rate {
>>>>           struct imx_sc_rpc_msg hdr;
>>>>           __le32 rate;
>>>>           __le16 resource;
>>>>           u8 clk;
>>>> -} __packed;
>>>> +} __packed __aligned(4);
>>>
>>> Sorry, this still doesn't make sense to me. Having __aligned(4) means
>>> that the struct is placed on the stack at some alignment, great, but it
>>> still has __packed so the sizeof this struct is some odd number like 11.
>>> If this struct is the last element on the stack it will end at some
>>> unaligned address and the mailbox code will read a few bytes beyond the
>>> end of the stack.
>>
>> I checked again and marking the struct with __aligned(4) makes it have
>> sizeof == 12 as intended. It was 11 before.
>>
>>       static_assert(sizeof(struct imx_sc_msg_req_set_clock_rate) == 12);
>>
>> After reading through your email and gcc docs again I'm not sure if this
>> portable/reliable this is but as far as I understand "sizeof" needs to
>> account for alignment. Or is this just an accident with my compiler?
>>
>> Marking a structure both __packed and __aligned(4) means that __packed
>> only affects internal struct member layout but sizeof is still rounded
>> up to a multiple of 4:
>>
>> struct test {
>>          u8      a;
>>          u16     b;
>> } __packed __aligned(4);
>>
>> static_assert(sizeof(struct test) == 4);
>> static_assert(offsetof(struct test, a) == 0);
>> static_assert(offsetof(struct test, b) == 1);
>>
>> This test is not realistic because I don't think SCU messages have any
>> such oddly-aligned members.
>>
> 
> I'm not really sure as I'm not a linker expert. I'm just especially wary
> of using __packed or __aligned attributes because they silently generate
> code that is usually inefficient. This is why we typically do lots of
> shifting and masking in the kernel, so that we can easily see how
> complicated it is to pack bits into place. Maybe it makes sense to get
> rid of the structs entirely and pack the bits into __le32 arrays of
> varying length. Then we don't have to worry about packed or aligned or
> what the compiler will do and we can easily be confident that we've put
> the bits in the right place in each u32 that is eventually written to
> the mailbox register space.

These message structs are not as complicated as hardware register, for 
example everything is always on a byte border.

In older versions of the imx internal tree SC messaging is done by 
packing into arrays through a layer of generated code which looks like this:

          RPC_VER(&msg) = SC_RPC_VERSION;
          RPC_SVC(&msg) = U8(SC_RPC_SVC_MISC);
          RPC_FUNC(&msg) = U8(MISC_FUNC_SET_CONTROL);
          RPC_U32(&msg, 0U) = U32(ctrl);
          RPC_U32(&msg, 4U) = U32(val);
          RPC_U16(&msg, 8U) = U16(resource);
          RPC_SIZE(&msg) = 4U;

The RPC_U32/U16 macros look like this:

#define RPC_I32(MESG, IDX)      ((MESG)->DATA.i32[(IDX) / 4U])
#define RPC_I16(MESG, IDX)      ((MESG)->DATA.i16[(IDX) / 2U])
#define RPC_I8(MESG, IDX)       ((MESG)->DATA.i8[(IDX)])
#define RPC_U32(MESG, IDX)      ((MESG)->DATA.u32[(IDX) / 4U])
#define RPC_U16(MESG, IDX)      ((MESG)->DATA.u16[(IDX) / 2U])
#define RPC_U8(MESG, IDX)       ((MESG)->DATA.u8[(IDX)])

and the message struct itself has a big union for the data:

typedef struct {
          uint8_t version;
          uint8_t size;
          uint8_t svc;
          uint8_t func;
          union {
                  int32_t i32[(SC_RPC_MAX_MSG - 1U)];
                  int16_t i16[(SC_RPC_MAX_MSG - 1U) * 2U];
                  int8_t i8[(SC_RPC_MAX_MSG - 1U) * 4U];
                  uint32_t u32[(SC_RPC_MAX_MSG - 1U)];
                  uint16_t u16[(SC_RPC_MAX_MSG - 1U) * 2U];
                  uint8_t u8[(SC_RPC_MAX_MSG - 1U) * 4U];
          } DATA;
} sc_rpc_msg_t;

This approach is very verbose to the point of being unreadable I think 
it's much to message structs instead. Compiler struct layout rules are 
not really all that complicated and casting binary data as structs is 
very common in areas such as networking. This approach is also used by 
other firmware interfaces like TI sci and nvidia bpmp.

imx8 currently has manually written message structs, it's unfortunate 
that a bug was found and fixing required a scattering patches in 
multiple subsystems. Perhaps a better solution would be to centralize 
all structs in a single header similar to drivers/firmware/ti_sci.h?

In order to ensrue that there are no issues specific to the compile 
version perhaps a bunch of static_assert statements could be added to 
check that sizeof and offset are as expected?

---------------------------------

As far as I can tell the issue KASAN warns about can be simplified to this:

struct __packed badpack {
     u32     a;
     u16     b;
     u8      c;
};

static_assert(sizeof(struct badpack) == 7);

static void func(void *x)
{
     u32* arr = (u32*)x;
     arr[0] = 0x11111111;
     arr[1] = 0x22222222;
}

static int hello(void)
{
     struct badpack s;
     u8 x = 0x33;

     printk("&s=%px &x=%px\n", &s, &x);
     func(&s);
     // x could be overwritten here, depending on stack layout.
     BUG_ON(x != 0x33);

     return 0;
}

Adding __aligned(4) bumps struct size to 8 and avoids the issue

Added KASAN maintainers to check if this is a valid fix.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4
  2020-03-17 19:25         ` Leonard Crestez
@ 2020-03-17 19:54           ` Dmitry Vyukov
  0 siblings, 0 replies; 23+ messages in thread
From: Dmitry Vyukov @ 2020-03-17 19:54 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Aisheng Dong, Alessandro Zummo, Alexandre Belloni, Abel Vesa,
	Anson Huang, Stephen Boyd, Michael Turquette, Stefan Agner,
	linux-clk, Franck Lenormand, Alexander Potapenko, dl-linux-imx,
	Fabio Estevam, Andrey Ryabinin, kasan-dev, Shawn Guo,
	Linus Walleij, linux-arm-kernel

On Tue, Mar 17, 2020 at 8:25 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 2020-02-27 3:48 AM, Stephen Boyd wrote:
> > Quoting Leonard Crestez (2020-02-25 11:52:11)
> >> On 25.02.2020 18:52, Stephen Boyd wrote:
> >>> Quoting Leonard Crestez (2020-02-20 08:29:32)
> >>>> The imx SC api strongly assumes that messages are composed out of
> >>>> 4-bytes words but some of our message structs have odd sizeofs.
> >>>>
> >>>> This produces many oopses with CONFIG_KASAN=y.
> >>>>
> >>>> Fix by marking with __aligned(4).
> >>>>
> >>>> Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>>> ---
> >>>>    drivers/clk/imx/clk-scu.c | 6 +++---
> >>>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> >>>> index fbef740704d0..3c5c42d8833e 100644
> >>>> --- a/drivers/clk/imx/clk-scu.c
> >>>> +++ b/drivers/clk/imx/clk-scu.c
> >>>> @@ -41,16 +41,16 @@ struct clk_scu {
> >>>>    struct imx_sc_msg_req_set_clock_rate {
> >>>>           struct imx_sc_rpc_msg hdr;
> >>>>           __le32 rate;
> >>>>           __le16 resource;
> >>>>           u8 clk;
> >>>> -} __packed;
> >>>> +} __packed __aligned(4);
> >>>
> >>> Sorry, this still doesn't make sense to me. Having __aligned(4) means
> >>> that the struct is placed on the stack at some alignment, great, but it
> >>> still has __packed so the sizeof this struct is some odd number like 11.
> >>> If this struct is the last element on the stack it will end at some
> >>> unaligned address and the mailbox code will read a few bytes beyond the
> >>> end of the stack.
> >>
> >> I checked again and marking the struct with __aligned(4) makes it have
> >> sizeof == 12 as intended. It was 11 before.
> >>
> >>       static_assert(sizeof(struct imx_sc_msg_req_set_clock_rate) == 12);
> >>
> >> After reading through your email and gcc docs again I'm not sure if this
> >> portable/reliable this is but as far as I understand "sizeof" needs to
> >> account for alignment. Or is this just an accident with my compiler?
> >>
> >> Marking a structure both __packed and __aligned(4) means that __packed
> >> only affects internal struct member layout but sizeof is still rounded
> >> up to a multiple of 4:
> >>
> >> struct test {
> >>          u8      a;
> >>          u16     b;
> >> } __packed __aligned(4);
> >>
> >> static_assert(sizeof(struct test) == 4);
> >> static_assert(offsetof(struct test, a) == 0);
> >> static_assert(offsetof(struct test, b) == 1);
> >>
> >> This test is not realistic because I don't think SCU messages have any
> >> such oddly-aligned members.
> >>
> >
> > I'm not really sure as I'm not a linker expert. I'm just especially wary
> > of using __packed or __aligned attributes because they silently generate
> > code that is usually inefficient. This is why we typically do lots of
> > shifting and masking in the kernel, so that we can easily see how
> > complicated it is to pack bits into place. Maybe it makes sense to get
> > rid of the structs entirely and pack the bits into __le32 arrays of
> > varying length. Then we don't have to worry about packed or aligned or
> > what the compiler will do and we can easily be confident that we've put
> > the bits in the right place in each u32 that is eventually written to
> > the mailbox register space.
>
> These message structs are not as complicated as hardware register, for
> example everything is always on a byte border.
>
> In older versions of the imx internal tree SC messaging is done by
> packing into arrays through a layer of generated code which looks like this:
>
>           RPC_VER(&msg) = SC_RPC_VERSION;
>           RPC_SVC(&msg) = U8(SC_RPC_SVC_MISC);
>           RPC_FUNC(&msg) = U8(MISC_FUNC_SET_CONTROL);
>           RPC_U32(&msg, 0U) = U32(ctrl);
>           RPC_U32(&msg, 4U) = U32(val);
>           RPC_U16(&msg, 8U) = U16(resource);
>           RPC_SIZE(&msg) = 4U;
>
> The RPC_U32/U16 macros look like this:
>
> #define RPC_I32(MESG, IDX)      ((MESG)->DATA.i32[(IDX) / 4U])
> #define RPC_I16(MESG, IDX)      ((MESG)->DATA.i16[(IDX) / 2U])
> #define RPC_I8(MESG, IDX)       ((MESG)->DATA.i8[(IDX)])
> #define RPC_U32(MESG, IDX)      ((MESG)->DATA.u32[(IDX) / 4U])
> #define RPC_U16(MESG, IDX)      ((MESG)->DATA.u16[(IDX) / 2U])
> #define RPC_U8(MESG, IDX)       ((MESG)->DATA.u8[(IDX)])
>
> and the message struct itself has a big union for the data:
>
> typedef struct {
>           uint8_t version;
>           uint8_t size;
>           uint8_t svc;
>           uint8_t func;
>           union {
>                   int32_t i32[(SC_RPC_MAX_MSG - 1U)];
>                   int16_t i16[(SC_RPC_MAX_MSG - 1U) * 2U];
>                   int8_t i8[(SC_RPC_MAX_MSG - 1U) * 4U];
>                   uint32_t u32[(SC_RPC_MAX_MSG - 1U)];
>                   uint16_t u16[(SC_RPC_MAX_MSG - 1U) * 2U];
>                   uint8_t u8[(SC_RPC_MAX_MSG - 1U) * 4U];
>           } DATA;
> } sc_rpc_msg_t;
>
> This approach is very verbose to the point of being unreadable I think
> it's much to message structs instead. Compiler struct layout rules are
> not really all that complicated and casting binary data as structs is
> very common in areas such as networking. This approach is also used by
> other firmware interfaces like TI sci and nvidia bpmp.
>
> imx8 currently has manually written message structs, it's unfortunate
> that a bug was found and fixing required a scattering patches in
> multiple subsystems. Perhaps a better solution would be to centralize
> all structs in a single header similar to drivers/firmware/ti_sci.h?
>
> In order to ensrue that there are no issues specific to the compile
> version perhaps a bunch of static_assert statements could be added to
> check that sizeof and offset are as expected?
>
> ---------------------------------
>
> As far as I can tell the issue KASAN warns about can be simplified to this:
>
> struct __packed badpack {
>      u32     a;
>      u16     b;
>      u8      c;
> };
>
> static_assert(sizeof(struct badpack) == 7);
>
> static void func(void *x)
> {
>      u32* arr = (u32*)x;
>      arr[0] = 0x11111111;
>      arr[1] = 0x22222222;
> }
>
> static int hello(void)
> {
>      struct badpack s;
>      u8 x = 0x33;
>
>      printk("&s=%px &x=%px\n", &s, &x);
>      func(&s);
>      // x could be overwritten here, depending on stack layout.
>      BUG_ON(x != 0x33);
>
>      return 0;
> }
>
> Adding __aligned(4) bumps struct size to 8 and avoids the issue
>
> Added KASAN maintainers to check if this is a valid fix.

Hi Leonard,

I think it should fix the bug.
It's not so much about KASAN, more about the validity of the C program.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-03-17 19:54 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 16:29 [PATCH v2 0/8] firmware: imx: Align imx SC msg structs to 4 Leonard Crestez
2020-02-20 16:29 ` [PATCH v2 1/8] clk: imx: Align imx sc clock " Leonard Crestez
2020-02-24  7:22   ` Shawn Guo
2020-03-16  0:25     ` Shawn Guo
2020-02-25 16:51   ` Stephen Boyd
2020-02-25 19:52     ` Leonard Crestez
     [not found]       ` <158276809953.177367.6095692240077023796@swboyd.mtv.corp.google.com>
2020-03-17 19:25         ` Leonard Crestez
2020-03-17 19:54           ` Dmitry Vyukov
2020-03-16  8:52     ` Aisheng Dong
2020-02-20 16:29 ` [PATCH v2 2/8] clk: imx: Align imx sc clock parent " Leonard Crestez
2020-02-24  7:23   ` Shawn Guo
2020-02-20 16:29 ` [PATCH v2 3/8] firmware: imx: misc: Align imx sc " Leonard Crestez
2020-02-24  7:28   ` Shawn Guo
2020-02-20 16:29 ` [PATCH v2 4/8] firmware: imx: scu-pd: " Leonard Crestez
2020-02-24  7:28   ` Shawn Guo
2020-02-20 16:29 ` [PATCH v2 5/8] firmware: imx: Align imx_sc_msg_req_cpu_start " Leonard Crestez
2020-02-24  7:28   ` Shawn Guo
2020-02-20 16:29 ` [PATCH v2 6/8] pinctrl: imx: scu: Align imx sc msg structs " Leonard Crestez
2020-02-21 15:34   ` Linus Walleij
2020-02-20 16:29 ` [PATCH v2 7/8] rtc: imx-sc: " Leonard Crestez
2020-03-03 11:02   ` Alexandre Belloni
2020-02-20 16:29 ` [PATCH v2 8/8] soc: imx-scu: " Leonard Crestez
2020-02-24  7:30   ` Shawn Guo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).