soc: imx: imx-scu: Getting UID from SCU should have response
diff mbox series

Message ID 1567624394-25023-1-git-send-email-Anson.Huang@nxp.com
State New
Headers show
Series
  • soc: imx: imx-scu: Getting UID from SCU should have response
Related show

Commit Message

Anson Huang Sept. 4, 2019, 7:13 p.m. UTC
The SCU firmware API for getting UID should have response,
otherwise, the message stored in function stack could be
released and then the response data received from SCU will be
stored into that released stack and cause kernel NULL pointer
dump.

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

Comments

Leonard Crestez Sept. 4, 2019, 7:34 a.m. UTC | #1
On 2019-09-04 10:14 AM, Anson Huang wrote:
> The SCU firmware API for getting UID should have response,
> otherwise, the message stored in function stack could be
> released and then the response data received from SCU will be
> stored into that released stack and cause kernel NULL pointer
> dump.

This fix looks good, but looking at imx-scu code it seems that passing 
the incorrect "have_resp" argument to imx_scu_call_rpc or just receiving 
an unexpected message from SCFW will always result in kernel stack 
corruption!

This is worth handling inside imx-scu itself: unless a response was 
explicitly requested it should ignore and print a warning on rx, for 
example by setting imx_sc_ipc to NULL when not waiting for a response.

Holding on to arbitrary stack pointers is bad.

--
Regards,
Leonard

> diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
> index 50831eb..c68882e 100644
> --- a/drivers/soc/imx/soc-imx-scu.c
> +++ b/drivers/soc/imx/soc-imx-scu.c
> @@ -46,7 +46,7 @@ static ssize_t soc_uid_show(struct device *dev,
>   	hdr->func = IMX_SC_MISC_FUNC_UNIQUE_ID;
>   	hdr->size = 1;
>   
> -	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, false);
> +	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
>   	if (ret) {
>   		pr_err("%s: get soc uid failed, ret %d\n", __func__, ret);
>   		return ret;
Anson Huang Sept. 4, 2019, 7:41 a.m. UTC | #2
Hi, Leonard

> On 2019-09-04 10:14 AM, Anson Huang wrote:
> > The SCU firmware API for getting UID should have response, otherwise,
> > the message stored in function stack could be released and then the
> > response data received from SCU will be stored into that released
> > stack and cause kernel NULL pointer dump.
> 
> This fix looks good, but looking at imx-scu code it seems that passing the
> incorrect "have_resp" argument to imx_scu_call_rpc or just receiving an
> unexpected message from SCFW will always result in kernel stack corruption!
> 
> This is worth handling inside imx-scu itself: unless a response was explicitly
> requested it should ignore and print a warning on rx, for example by setting
> imx_sc_ipc to NULL when not waiting for a response.
> 
> Holding on to arbitrary stack pointers is bad.

We noticed this issue recently during the development of ON/OFF button support,
this UID is lucky, the stack is NOT released when SCU response data is received, but
this fix should be applied.

We talked to Chuck about adding return value for these special APIs, response data
needed but no return value from SCU, so very likely we will need a patch in imx_sc_ipc
driver to enhance/handle that, will do a patch for it later.

Thanks,
Anson
Shawn Guo Oct. 6, 2019, 1:21 a.m. UTC | #3
On Wed, Sep 04, 2019 at 03:13:14PM -0400, Anson Huang wrote:
> The SCU firmware API for getting UID should have response,
> otherwise, the message stored in function stack could be
> released and then the response data received from SCU will be
> stored into that released stack and cause kernel NULL pointer
> dump.
> 
> Fixes: 73feb4d0f8f1 ("soc: imx-scu: Add SoC UID(unique identifier) support")
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Applied, thanks.

Patch
diff mbox series

diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
index 50831eb..c68882e 100644
--- a/drivers/soc/imx/soc-imx-scu.c
+++ b/drivers/soc/imx/soc-imx-scu.c
@@ -46,7 +46,7 @@  static ssize_t soc_uid_show(struct device *dev,
 	hdr->func = IMX_SC_MISC_FUNC_UNIQUE_ID;
 	hdr->size = 1;
 
-	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, false);
+	ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
 	if (ret) {
 		pr_err("%s: get soc uid failed, ret %d\n", __func__, ret);
 		return ret;