[v2] firmware: imx: scu: Fix corruption of header

Commit Message

Leonard Crestez March 25, 2020, 10 p.m. UTC

From: Franck LENORMAND <[e-mail]>

The header of the message to send can be changed if the
response is longer than the request:
 - 1st word, the header is sent
 - the remaining words of the message are sent
 - the response is received asynchronously during the
   execution of the loop, changing the size field in
   the header
 - the for loop test the termination condition using
   the corrupted header

It is the case for the API build_info which has just a
header as request but 3 words in response.

This issue is fixed storing the header locally instead of
using a pointer on it.

Fixes: edbee095fafb (firmware: imx: add SCU firmware driver support)

Signed-off-by: Franck LENORMAND <[e-mail]>
Reviewed-by: Leonard Crestez <[e-mail]>
Signed-off-by: Leonard Crestez <[e-mail]>
Cc: [e-mail]

---
Changes since v1:
* Add my signed-off-by as requested:
Link to v1: https://patchwork.kernel.org/patch/11394401/

There is another longer series attempting a fix but that probably won't
make it into stable:
https://patchwork.kernel.org/cover/11446661/

That series implements a different imx mailbox type but still maintains
the old path for DT compatibility so this fix is worth including. It
fixes real boot hangs.

 drivers/firmware/imx/imx-scu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Dong Aisheng March 26, 2020, 3:13 a.m. UTC | #1

> From: Leonard Crestez <[e-mail]>
> 
> The header of the message to send can be changed if the response is longer
> than the request:
>  - 1st word, the header is sent
>  - the remaining words of the message are sent
>  - the response is received asynchronously during the
>    execution of the loop, changing the size field in
>    the header
>  - the for loop test the termination condition using
>    the corrupted header
> 
> It is the case for the API build_info which has just a header as request but 3
> words in response.
> 
> This issue is fixed storing the header locally instead of using a pointer on it.
> 
> Fixes: edbee095fafb (firmware: imx: add SCU firmware driver support)
> 
> Signed-off-by: Franck LENORMAND <[e-mail]>
> Reviewed-by: Leonard Crestez <[e-mail]>
> Signed-off-by: Leonard Crestez <[e-mail]>
> Cc: [e-mail]

Reviewed-by: Dong Aisheng <[e-mail]>

Regards
Aisheng

> 
> ---
> Changes since v1:
> * Add my signed-off-by as requested:
> Link to v1:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw
> ork.kernel.org%2Fpatch%2F11394401%2F&amp;data=02%7C01%7Caisheng.d
> ong%40nxp.com%7C67d37a0e46c4473ec6f808d7d107e780%7C686ea1d3bc
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C637207704182589926&amp;sdata
> =Qzj2kFXpN%2FfO%2BgclQ1RcQDmwlLUTuLdg78WxWgTy4cc%3D&amp;reser
> ved=0
> 
> There is another longer series attempting a fix but that probably won't make it
> into stable:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw
> ork.kernel.org%2Fcover%2F11446661%2F&amp;data=02%7C01%7Caisheng.d
> ong%40nxp.com%7C67d37a0e46c4473ec6f808d7d107e780%7C686ea1d3bc
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C637207704182599918&amp;sdata
> =mBJ8fX4DzU0JArPobbCf1GRvupEo0uL9hV4Htf6kN3I%3D&amp;reserved=0
> 
> That series implements a different imx mailbox type but still maintains the old
> path for DT compatibility so this fix is worth including. It fixes real boot hangs.
> 
>  drivers/firmware/imx/imx-scu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index f71eaa5bf52d..7119228a8059 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -141,24 +141,24 @@ static void imx_scu_rx_callback(struct mbox_client
> *c, void *msg)
>  		complete(&sc_ipc->done);
>  }
> 
>  static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg)  {
> -	struct imx_sc_rpc_msg *hdr = msg;
> +	struct imx_sc_rpc_msg hdr = *(struct imx_sc_rpc_msg *)msg;
>  	struct imx_sc_chan *sc_chan;
>  	u32 *data = msg;
>  	int ret;
>  	int i;
> 
>  	/* Check size */
> -	if (hdr->size > IMX_SC_RPC_MAX_MSG)
> +	if (hdr.size > IMX_SC_RPC_MAX_MSG)
>  		return -EINVAL;
> 
> -	dev_dbg(sc_ipc->dev, "RPC SVC %u FUNC %u SIZE %u\n", hdr->svc,
> -		hdr->func, hdr->size);
> +	dev_dbg(sc_ipc->dev, "RPC SVC %u FUNC %u SIZE %u\n", hdr.svc,
> +		hdr.func, hdr.size);
> 
> -	for (i = 0; i < hdr->size; i++) {
> +	for (i = 0; i < hdr.size; i++) {
>  		sc_chan = &sc_ipc->chans[i % 4];
> 
>  		/*
>  		 * SCU requires that all messages words are written
>  		 * sequentially but linux MU driver implements multiple
> --
> 2.17.1

Shawn Guo April 23, 2020, 2:35 p.m. UTC | #2

On Thu, Mar 26, 2020 at 12:00:05AM +0200, Leonard Crestez wrote:
> From: Franck LENORMAND <[e-mail]>
> 
> The header of the message to send can be changed if the
> response is longer than the request:
>  - 1st word, the header is sent
>  - the remaining words of the message are sent
>  - the response is received asynchronously during the
>    execution of the loop, changing the size field in
>    the header
>  - the for loop test the termination condition using
>    the corrupted header
> 
> It is the case for the API build_info which has just a
> header as request but 3 words in response.
> 
> This issue is fixed storing the header locally instead of
> using a pointer on it.
> 
> Fixes: edbee095fafb (firmware: imx: add SCU firmware driver support)
> 
> Signed-off-by: Franck LENORMAND <[e-mail]>
> Reviewed-by: Leonard Crestez <[e-mail]>
> Signed-off-by: Leonard Crestez <[e-mail]>
> Cc: [e-mail]

Applied, thanks.

Patch

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index f71eaa5bf52d..7119228a8059 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -141,24 +141,24 @@  static void imx_scu_rx_callback(struct mbox_client *c, void *msg)
 		complete(&sc_ipc->done);
 }
 
 static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg)
 {
-	struct imx_sc_rpc_msg *hdr = msg;
+	struct imx_sc_rpc_msg hdr = *(struct imx_sc_rpc_msg *)msg;
 	struct imx_sc_chan *sc_chan;
 	u32 *data = msg;
 	int ret;
 	int i;
 
 	/* Check size */
-	if (hdr->size > IMX_SC_RPC_MAX_MSG)
+	if (hdr.size > IMX_SC_RPC_MAX_MSG)
 		return -EINVAL;
 
-	dev_dbg(sc_ipc->dev, "RPC SVC %u FUNC %u SIZE %u\n", hdr->svc,
-		hdr->func, hdr->size);
+	dev_dbg(sc_ipc->dev, "RPC SVC %u FUNC %u SIZE %u\n", hdr.svc,
+		hdr.func, hdr.size);
 
-	for (i = 0; i < hdr->size; i++) {
+	for (i = 0; i < hdr.size; i++) {
 		sc_chan = &sc_ipc->chans[i % 4];
 
 		/*
 		 * SCU requires that all messages words are written
 		 * sequentially but linux MU driver implements multiple