[PATCH] firmware: imx: Align imx SC msg structs to 4

[PATCH] firmware: imx: Align imx SC msg structs to 4

[Leonard Crestez]

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.

Cc: stable@vger.kernel.org
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reported-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
 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(-)

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index fbef740704d0..b8b2072742a5 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;
 };
 
@@ -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;
 };
@@ -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);
 }
diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
index 4b56a587dacd..d073cb3ce699 100644
--- a/drivers/firmware/imx/misc.c
+++ b/drivers/firmware/imx/misc.c
@@ -14,30 +14,30 @@
 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;
 	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;
-} __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
diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
index b556612207e5..af3ae0087de4 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];
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;
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;
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

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Shawn Guo]

On Tue, Feb 11, 2020 at 11:24: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 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.
> 
> Cc: stable@vger.kernel.org

Should we have a fixes tag and send it for -rc?

Shawn

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reported-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---
>  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(-)
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> index fbef740704d0..b8b2072742a5 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;
>  };
>  
> @@ -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;
>  };
> @@ -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);
>  }
> diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
> index 4b56a587dacd..d073cb3ce699 100644
> --- a/drivers/firmware/imx/misc.c
> +++ b/drivers/firmware/imx/misc.c
> @@ -14,30 +14,30 @@
>  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;
>  	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;
> -} __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
> diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
> index b556612207e5..af3ae0087de4 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];
> 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;
> 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;
> 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
> 

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Leonard Crestez]

On 17.02.2020 08:21, Shawn Guo wrote:
> On Tue, Feb 11, 2020 at 11:24: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 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.
>>
>> Cc: stable@vger.kernel.org
> 
> Should we have a fixes tag and send it for -rc?

I haven't check but this would probably have to be split into multiple 
patches because the structs were not added all at once.

> Shawn
> 
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reported-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>>   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(-)
>>
>> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
>> index fbef740704d0..b8b2072742a5 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;
>>   };
>>   
>> @@ -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;
>>   };
>> @@ -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);
>>   }
>> diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
>> index 4b56a587dacd..d073cb3ce699 100644
>> --- a/drivers/firmware/imx/misc.c
>> +++ b/drivers/firmware/imx/misc.c
>> @@ -14,30 +14,30 @@
>>   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;
>>   	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;
>> -} __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
>> diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
>> index b556612207e5..af3ae0087de4 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];
>> 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;
>> 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;
>> 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
>>
> 

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Shawn Guo]

On Mon, Feb 17, 2020 at 08:37:45PM +0000, Leonard Crestez wrote:
> On 17.02.2020 08:21, Shawn Guo wrote:
> > On Tue, Feb 11, 2020 at 11:24: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 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.
> >>
> >> Cc: stable@vger.kernel.org
> > 
> > Should we have a fixes tag and send it for -rc?
> 
> I haven't check but this would probably have to be split into multiple 
> patches because the structs were not added all at once.

Or maybe we can just drop the stable tag, as it addresses a corner
case issue which could concern very few people?

Shawn

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Leonard Crestez]

On 18.02.2020 11:18, Shawn Guo wrote:
> On Mon, Feb 17, 2020 at 08:37:45PM +0000, Leonard Crestez wrote:
>> On 17.02.2020 08:21, Shawn Guo wrote:
>>> On Tue, Feb 11, 2020 at 11:24: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 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.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>
>>> Should we have a fixes tag and send it for -rc?
>>
>> I haven't check but this would probably have to be split into multiple
>> patches because the structs were not added all at once.
> 
> Or maybe we can just drop the stable tag, as it addresses a corner
> case issue which could concern very few people?

I think that "kernel does not boot with KASAN=y" is an issue worth fixing.

I will split and resend with appropriate Fixes: tags.

It seems likely that this will be picked up for -stable anyway via 
Sasha's automation scripts and those scripts benefit from Fixes: tags.

--
Regards,
Leonard

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Sasha Levin]

On Tue, Feb 18, 2020 at 05:48:50PM +0000, Leonard Crestez wrote:
>On 18.02.2020 11:18, Shawn Guo wrote:
>> On Mon, Feb 17, 2020 at 08:37:45PM +0000, Leonard Crestez wrote:
>>> On 17.02.2020 08:21, Shawn Guo wrote:
>>>> On Tue, Feb 11, 2020 at 11:24: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 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.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>
>>>> Should we have a fixes tag and send it for -rc?
>>>
>>> I haven't check but this would probably have to be split into multiple
>>> patches because the structs were not added all at once.
>>
>> Or maybe we can just drop the stable tag, as it addresses a corner
>> case issue which could concern very few people?
>
>I think that "kernel does not boot with KASAN=y" is an issue worth fixing.
>
>I will split and resend with appropriate Fixes: tags.
>
>It seems likely that this will be picked up for -stable anyway via
>Sasha's automation scripts and those scripts benefit from Fixes: tags.

Even if not, we realy very much on KASAN working on stable kernels, so
please do fix this :)

-- 
Thanks,
Sasha

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Alexandre Belloni]

On 11/02/2020 23:24:33+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 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reported-by: Iuliana Prodan <iuliana.prodan@nxp.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  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(-)
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> index fbef740704d0..b8b2072742a5 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;
>  };
>  
> @@ -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;
>  };
> @@ -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);
>  }
> diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
> index 4b56a587dacd..d073cb3ce699 100644
> --- a/drivers/firmware/imx/misc.c
> +++ b/drivers/firmware/imx/misc.c
> @@ -14,30 +14,30 @@
>  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;
>  	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;
> -} __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
> diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
> index b556612207e5..af3ae0087de4 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];
> 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;
> 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;
> 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
> 

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

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Stephen Boyd]

Quoting Leonard Crestez (2020-02-11 13:24:33)
> 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

Can you share the full kasan bug report instead of the single line?

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

Is packed necessary for these? I thought that if the beginning of the
struct was naturally aligned and there was sometimes a byte or two at
the end then having __packed wasn't useful. So maybe it's better to just
drop __packed on all these structs and let the compiler decide it can
add some padding on the stack? Or do we have arrays of these structs
sitting in memory all next to each other and they need to be that way to
be sent through the mailbox?

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reported-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---

Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Leonard Crestez]

On 20.02.2020 01:57, Stephen Boyd wrote:
> Quoting Leonard Crestez (2020-02-11 13:24:33)
>> 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
> 
> Can you share the full kasan bug report instead of the single line?

[    1.606708] imx-scu scu: NXP i.MX SCU Initialized
[    1.635265] random: fast init done
[    1.652200] 
==================================================================
[    1.659118] BUG: KASAN: stack-out-of-bounds in 
imx_mu_send_data+0x108/0x1f0
[    1.666046] Read of size 4 at addr ffff0008c80e6bc4 by task swapper/0/1
[    1.672642]
[    1.674134] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
5.4.3-03848-g13efcd6 #54
[    1.681335] Hardware name: Freescale i.MX8QM MEK (DT)
[    1.686373] Call trace:
[    1.688815]  dump_backtrace+0x0/0x1e8
[    1.692458]  show_stack+0x14/0x20
[    1.695766]  dump_stack+0xe8/0x140
[    1.699155]  print_address_description.isra.11+0x64/0x348
[    1.704532]  __kasan_report+0x11c/0x230
[    1.708356]  kasan_report+0xc/0x18
[    1.711743]  __asan_load4+0x90/0xb0
[    1.715218]  imx_mu_send_data+0x108/0x1f0
[    1.719215]  msg_submit+0x104/0x180
[    1.722689]  mbox_send_message+0xa8/0x1a0
[    1.726696]  imx_scu_call_rpc+0x168/0x310
[    1.730679]  imx_sc_pd_power+0x180/0x1e0
[    1.734589]  imx_sc_pd_power_on+0x10/0x18
[    1.738598]  genpd_power_on.part.23+0x118/0x2a8
[    1.743105]  genpd_runtime_resume+0x138/0x320
[    1.747454]  __rpm_callback+0xb0/0x1a0
[    1.751184]  rpm_callback+0x34/0xe0
[    1.754659]  rpm_resume+0x5b8/0x7e8
[    1.758137]  __pm_runtime_resume+0x38/0x90
[    1.762222]  imx_clk_scu_probe+0x5c/0x1c8
[    1.766218]  platform_drv_probe+0x6c/0xc8
[    1.770217]  really_probe+0x148/0x428
[    1.773861]  driver_probe_device+0x74/0x130
[    1.778031]  __device_attach_driver+0xc4/0xe8
[    1.782380]  bus_for_each_drv+0xf0/0x158
[    1.786283]  __device_attach+0x158/0x1d8
[    1.790195]  device_initial_probe+0x10/0x18
[    1.794362]  bus_probe_device+0xe0/0xf0
[    1.798185]  device_add+0x660/0x998
[    1.801659]  platform_device_add+0x198/0x340
[    1.805916]  imx_clk_scu_alloc_dev+0x1b8/0x1e8
[    1.810347]  imx8qxp_clk_probe+0x19d0/0x28b8
[    1.814601]  platform_drv_probe+0x6c/0xc8
[    1.818601]  really_probe+0x148/0x428
[    1.822250]  driver_probe_device+0x74/0x130
[    1.826423]  __device_attach_driver+0xc4/0xe8
[    1.830763]  bus_for_each_drv+0xf0/0x158
[    1.834675]  __device_attach+0x158/0x1d8
[    1.838583]  device_initial_probe+0x10/0x18
[    1.842752]  bus_probe_device+0xe0/0xf0
[    1.846572]  device_add+0x660/0x998
[    1.850058]  of_device_add+0x74/0x98
[    1.853610]  of_platform_device_create_pdata+0x11c/0x178
[    1.858908]  of_platform_bus_create+0x404/0x4f0
[    1.863425]  of_platform_populate+0x74/0x110
[    1.867688]  devm_of_platform_populate+0x54/0xb8
[    1.872291]  imx_scu_probe+0x1b8/0x220
[    1.876022]  platform_drv_probe+0x6c/0xc8
[    1.880021]  really_probe+0x148/0x428
[    1.883671]  driver_probe_device+0x74/0x130
[    1.887841]  device_driver_attach+0x94/0xa0
[    1.892010]  __driver_attach+0x70/0x110
[    1.895832]  bus_for_each_dev+0xe8/0x158
[    1.899741]  driver_attach+0x30/0x40
[    1.903303]  bus_add_driver+0x1b0/0x2b8
[    1.907129]  driver_register+0xbc/0x1d0
[    1.910947]  __platform_driver_register+0x7c/0x88
[    1.915653]  imx_scu_driver_init+0x18/0x20
[    1.919725]  do_one_initcall+0xd4/0x244
[    1.923552]  kernel_init_freeable+0x238/0x2d4
[    1.927889]  kernel_init+0x10/0x114
[    1.931365]  ret_from_fork+0x10/0x18
[    1.934917]
[    1.936399] The buggy address belongs to the page:
[    1.941184] page:fffffe0023003980 refcount:0 mapcount:0 
mapping:0000000000000000 index:0x0
[    1.949447] raw: 1ffff00000000000 fffffe0023003988 fffffe0023003988 
0000000000000000
[    1.957170] raw: 0000000000000000 0000000000000000 00000000ffffffff 
0000000000000000
[    1.964894] page dumped because: kasan: bad access detected
[    1.970449]
[    1.971933] addr ffff0008c80e6bc4 is located in stack of task 
swapper/0/1 at offset 36 in frame:
[    1.980708]  imx_sc_pd_power+0x0/0x1e0
[    1.984442]
[    1.985917] this frame has 1 object:
[    1.989481]  [32, 39) 'msg'
[    1.989484]
[    1.993732] Memory state around the buggy address:
[    1.998520]  ffff0008c80e6a80: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 
f1 00 00
[    2.005730]  ffff0008c80e6b00: 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 
00 00 00
[    2.012940] >ffff0008c80e6b80: 00 00 00 00 f1 f1 f1 f1 07 f2 f2 f2 00 
00 00 00
[    2.020141]                                            ^
[    2.025448]  ffff0008c80e6c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[    2.032659]  ffff0008c80e6c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[    2.039862] 
==================================================================

This is actually from an NXP release branch but code is very close up 
upstream.

>> It shouldn't cause an issues in normal use because these structs are
>> always allocated on the stack.
> 
> Is packed necessary for these? I thought that if the beginning of the
> struct was naturally aligned and there was sometimes a byte or two at
> the end then having __packed wasn't useful. So maybe it's better to just
> drop __packed on all these structs and let the compiler decide it can
> add some padding on the stack? Or do we have arrays of these structs
> sitting in memory all next to each other and they need to be that way to
> be sent through the mailbox?

I'm not sure I understand the question: the structs are __packed because 
they represent the binary protocol for communicating with the "System 
Controller".

Without __packed the compiler could insert padding inside the structs 
and break the protocol.

As far as I understand compilers are still allowed to use padding on 
stack since that padding is outside the message struct itself.

--
Regards,
Leonard