All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] firmware: imx: warn on unexpected RX
@ 2019-09-04  7:54 Leonard Crestez
  2019-09-04  8:00 ` Anson Huang
  2019-10-06  0:57 ` Shawn Guo
  0 siblings, 2 replies; 3+ messages in thread
From: Leonard Crestez @ 2019-09-04  7:54 UTC (permalink / raw)
  To: Anson Huang, Dong Aisheng, Shawn Guo
  Cc: Fabio Estevam, Jassi Brar, linux-imx, kernel, linux-arm-kernel

The imx_scu_call_rpc function function returns the result inside the
same "msg" struct containing the transmitted message. This is
implemented by holding a pointer to msg (which is usually on the stack)
in sc_imx_rpc and writing to it from imx_scu_rx_callback.

This means that if the have_resp parameter is incorrect or SCU sends an
unexpected for any reason the most likely result is kernel stack
corruption.

Fix this by only setting sc_imx_rpc.msg for the duration of the
imx_scu_call_rpc call and warning in imx_scu_rx_callback if unset.

Print the unexpected response data to help debugging.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/firmware/imx/imx-scu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 04a24a863d6e..869be7a5172c 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -105,10 +105,16 @@ static void imx_scu_rx_callback(struct mbox_client *c, void *msg)
 	struct imx_sc_chan *sc_chan = container_of(c, struct imx_sc_chan, cl);
 	struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
 	struct imx_sc_rpc_msg *hdr;
 	u32 *data = msg;
 
+	if (!sc_ipc->msg) {
+		dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x, ignore!\n",
+				sc_chan->idx, *data);
+		return;
+	}
+
 	if (sc_chan->idx == 0) {
 		hdr = msg;
 		sc_ipc->rx_size = hdr->size;
 		dev_dbg(sc_ipc->dev, "msg rx size %u\n", sc_ipc->rx_size);
 		if (sc_ipc->rx_size > 4)
@@ -163,11 +169,12 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
 		return -EINVAL;
 
 	mutex_lock(&sc_ipc->lock);
 	reinit_completion(&sc_ipc->done);
 
-	sc_ipc->msg = msg;
+	if (have_resp)
+		sc_ipc->msg = msg;
 	sc_ipc->count = 0;
 	ret = imx_scu_ipc_write(sc_ipc, msg);
 	if (ret < 0) {
 		dev_err(sc_ipc->dev, "RPC send msg failed: %d\n", ret);
 		goto out;
@@ -185,10 +192,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
 		hdr = msg;
 		ret = hdr->func;
 	}
 
 out:
+	sc_ipc->msg = NULL;
 	mutex_unlock(&sc_ipc->lock);
 
 	dev_dbg(sc_ipc->dev, "RPC SVC done\n");
 
 	return imx_sc_to_linux_errno(ret);
-- 
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] 3+ messages in thread
* RE: [PATCH] firmware: imx: warn on unexpected RX
  2019-09-04  7:54 [PATCH] firmware: imx: warn on unexpected RX Leonard Crestez
@ 2019-09-04  8:00 ` Anson Huang
  2019-10-06  0:57 ` Shawn Guo
  1 sibling, 0 replies; 3+ messages in thread
From: Anson Huang @ 2019-09-04  8:00 UTC (permalink / raw)
  To: Leonard Crestez, Aisheng Dong, Shawn Guo
  Cc: Fabio Estevam, Jassi Brar, dl-linux-imx, kernel, linux-arm-kernel



> Subject: [PATCH] firmware: imx: warn on unexpected RX
> 
> The imx_scu_call_rpc function function returns the result inside the same
> "msg" struct containing the transmitted message. This is implemented by
> holding a pointer to msg (which is usually on the stack) in sc_imx_rpc and
> writing to it from imx_scu_rx_callback.
> 
> This means that if the have_resp parameter is incorrect or SCU sends an
> unexpected for any reason the most likely result is kernel stack corruption.
> 
> Fix this by only setting sc_imx_rpc.msg for the duration of the
> imx_scu_call_rpc call and warning in imx_scu_rx_callback if unset.
> 
> Print the unexpected response data to help debugging.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Acked-by: Anson Huang <Anson.Huang@nxp.com>

> ---
>  drivers/firmware/imx/imx-scu.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> scu.c index 04a24a863d6e..869be7a5172c 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -105,10 +105,16 @@ static void imx_scu_rx_callback(struct mbox_client
> *c, void *msg)
>  	struct imx_sc_chan *sc_chan = container_of(c, struct imx_sc_chan,
> cl);
>  	struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
>  	struct imx_sc_rpc_msg *hdr;
>  	u32 *data = msg;
> 
> +	if (!sc_ipc->msg) {
> +		dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x,
> ignore!\n",
> +				sc_chan->idx, *data);
> +		return;
> +	}
> +
>  	if (sc_chan->idx == 0) {
>  		hdr = msg;
>  		sc_ipc->rx_size = hdr->size;
>  		dev_dbg(sc_ipc->dev, "msg rx size %u\n", sc_ipc->rx_size);
>  		if (sc_ipc->rx_size > 4)
> @@ -163,11 +169,12 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc,
> void *msg, bool have_resp)
>  		return -EINVAL;
> 
>  	mutex_lock(&sc_ipc->lock);
>  	reinit_completion(&sc_ipc->done);
> 
> -	sc_ipc->msg = msg;
> +	if (have_resp)
> +		sc_ipc->msg = msg;
>  	sc_ipc->count = 0;
>  	ret = imx_scu_ipc_write(sc_ipc, msg);
>  	if (ret < 0) {
>  		dev_err(sc_ipc->dev, "RPC send msg failed: %d\n", ret);
>  		goto out;
> @@ -185,10 +192,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc,
> void *msg, bool have_resp)
>  		hdr = msg;
>  		ret = hdr->func;
>  	}
> 
>  out:
> +	sc_ipc->msg = NULL;
>  	mutex_unlock(&sc_ipc->lock);
> 
>  	dev_dbg(sc_ipc->dev, "RPC SVC done\n");
> 
>  	return imx_sc_to_linux_errno(ret);
> --
> 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] 3+ messages in thread
* Re: [PATCH] firmware: imx: warn on unexpected RX
  2019-09-04  7:54 [PATCH] firmware: imx: warn on unexpected RX Leonard Crestez
  2019-09-04  8:00 ` Anson Huang
@ 2019-10-06  0:57 ` Shawn Guo
  1 sibling, 0 replies; 3+ messages in thread
From: Shawn Guo @ 2019-10-06  0:57 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, Anson Huang, Jassi Brar, linux-imx, kernel,
	Fabio Estevam, linux-arm-kernel

On Wed, Sep 04, 2019 at 10:54:58AM +0300, Leonard Crestez wrote:
> The imx_scu_call_rpc function function returns the result inside the

s/function function/function

> same "msg" struct containing the transmitted message. This is
> implemented by holding a pointer to msg (which is usually on the stack)
> in sc_imx_rpc and writing to it from imx_scu_rx_callback.
> 
> This means that if the have_resp parameter is incorrect or SCU sends an
> unexpected for any reason the most likely result is kernel stack

unexpected response

I fixed them up and applied the patch.

Shawn

> corruption.
> 
> Fix this by only setting sc_imx_rpc.msg for the duration of the
> imx_scu_call_rpc call and warning in imx_scu_rx_callback if unset.
> 
> Print the unexpected response data to help debugging.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/firmware/imx/imx-scu.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 04a24a863d6e..869be7a5172c 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -105,10 +105,16 @@ static void imx_scu_rx_callback(struct mbox_client *c, void *msg)
>  	struct imx_sc_chan *sc_chan = container_of(c, struct imx_sc_chan, cl);
>  	struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
>  	struct imx_sc_rpc_msg *hdr;
>  	u32 *data = msg;
>  
> +	if (!sc_ipc->msg) {
> +		dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x, ignore!\n",
> +				sc_chan->idx, *data);
> +		return;
> +	}
> +
>  	if (sc_chan->idx == 0) {
>  		hdr = msg;
>  		sc_ipc->rx_size = hdr->size;
>  		dev_dbg(sc_ipc->dev, "msg rx size %u\n", sc_ipc->rx_size);
>  		if (sc_ipc->rx_size > 4)
> @@ -163,11 +169,12 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
>  		return -EINVAL;
>  
>  	mutex_lock(&sc_ipc->lock);
>  	reinit_completion(&sc_ipc->done);
>  
> -	sc_ipc->msg = msg;
> +	if (have_resp)
> +		sc_ipc->msg = msg;
>  	sc_ipc->count = 0;
>  	ret = imx_scu_ipc_write(sc_ipc, msg);
>  	if (ret < 0) {
>  		dev_err(sc_ipc->dev, "RPC send msg failed: %d\n", ret);
>  		goto out;
> @@ -185,10 +192,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
>  		hdr = msg;
>  		ret = hdr->func;
>  	}
>  
>  out:
> +	sc_ipc->msg = NULL;
>  	mutex_unlock(&sc_ipc->lock);
>  
>  	dev_dbg(sc_ipc->dev, "RPC SVC done\n");
>  
>  	return imx_sc_to_linux_errno(ret);
> -- 
> 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] 3+ messages in thread
end of thread, other threads:[~2019-10-06  0:57 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04  7:54 [PATCH] firmware: imx: warn on unexpected RX Leonard Crestez
2019-09-04  8:00 ` Anson Huang
2019-10-06  0:57 ` Shawn Guo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.