Introduce boot warning print for vanilla developer builds #4987

jbech-linaro · 2021-11-18T14:52:35Z

Recent security issues reported to the OP-TEE project covering iMX-devices has mostly been about current CSU configuration in the OP-TEE project. This purpose of this PR is to reduce the likelihood of configuring plat-imx incorrectly. It's simply a debug print. It might seem overkill to have something like this, but I think we've seen enough reports now that we can justify adding this.

jbech-linaro · 2021-11-18T14:53:07Z

core/arch/arm/plat-imx/drivers/imx_csu.c

@@ -103,6 +103,9 @@ static TEE_Result csu_init(void)
 	const struct csu_config *csu_config = NULL;
 	const struct csu_setting *csu_setting = NULL;

+	if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))
+		IMSG("Device running with an insecure setting, consult the security reference manual from NXP");


I believe we allow long debug print lines since a while back?

May I suggest something like:
IMSG("Device running with an open security settings, please refer to OP-TEE i.MX platform documentation"); ?
OPTEE i.MX documentation being the readthedocs page where NXP will add the disclaimer.

Edit: In the end, I agree with @jforissier and @jenswi-linaro. I'm pretty sure other platforms feature equivalent mechanisms. Even optee core has some default insecure configuration (as mentionned for default_ta.pem). Would a global warning for all platforms make more sense?
This warning would redirect people to the documentation for additional information.

jbech-linaro · 2021-11-18T14:53:58Z

core/arch/arm/plat-imx/conf.mk

+# For more information about some of the implications when not setting this
+# correctly can be found at:
+#   - https://github.com/OP-TEE/optee_os/security/advisories/GHSA-6q85-3ph3-rm47
+#   - https://github.com/OP-TEE/optee_os/security/advisories/GHSA-4pqr-q8rf-8464


This URL is currently only working for OP-TEE admins. But once we've moved it to "public" it will work for anyone.

jforissier

Like you (probably) I'd like we could do better, but I have nothing to suggest. This all looks reasonable and acceptable to me, so:

Acked-by: Jerome Forissier <jerome@forissier.org>

Pushing a bit further... shouldn't a similar message be printed for all devices? As you said, issues were reported related to the IMX CSU, but different platforms have equivalent mechanisms. Or you want to be able to point the downstream user to specific things?

jforissier · 2021-11-18T15:03:54Z

core/arch/arm/plat-imx/drivers/imx_csu.c

+	if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))
+		IMSG("Device running with an insecure setting, consult the security reference manual from NXP");


Yes I think there was a consensus. That being said, you could as well print two lines:

Suggested change

if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))

IMSG("Device running with an insecure setting, consult the security reference manual from NXP");

if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING)) {

IMSG("Device running with an insecure setting!");

IMSG("Consult the security reference manual from NXP");

}

Would "possibly running" be more accurate?

I don't really see how this helps. The embedded core image may contain mis-configured security elements with or w/o the switch enabled.
From a platform user perspective, I think I would expect platform refuses to boot or provide some services when CFG_INSECURE_SETTINGS=n and something is badly configured.

etienne-lms · 2021-11-18T15:08:43Z

core/arch/arm/plat-imx/conf.mk

+#     accordingly. NXP shares the security reference manual with their partners
+#     under a NDA. I.e., partners will need to sign a NDA with NXP to get hold
+#     of the security reference manual. Because of this the OP-TEE project
+#     cannot and are not allowed to share the security reference manual.


etienne-lms · 2021-11-18T15:18:14Z

core/arch/arm/plat-imx/drivers/imx_csu.c

+	if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))
+		IMSG("Device running with an insecure setting, consult the security reference manual from NXP");


I don't really see how this helps. The embedded core image may contain mis-configured security elements with or w/o the switch enabled.
From a platform user perspective, I think I would expect platform refuses to boot or provide some services when CFG_INSECURE_SETTINGS=n and something is badly configured.

jenswi-linaro · 2021-11-18T15:29:32Z

Pushing a bit further... shouldn't a similar message be printed for all devices?

Agree, that would include to use keys/default_ta.pem for instance.

clementfaure

Hi @jbech-linaro, thanks for your help.
Please see my comments

clementfaure · 2021-11-18T15:18:33Z

core/arch/arm/plat-imx/conf.mk

@@ -460,5 +460,30 @@ CFG_IMX_CAAM ?= y
 endif
 endif

+# This is only here to force an informative print showing that the device runs


I would wait for NXP to publish the disclaimer before adding this chunk of text. Some information added in this file will also be specified in the disclaimer NXP is currently working on.

I would also personally prefer to have explanatory text in the documentation rather than in the source code.
Let me know if waiting for the disclaimer before merging is okay for you.

clementfaure · 2021-11-18T15:24:36Z

core/arch/arm/plat-imx/drivers/imx_csu.c

@@ -103,6 +103,9 @@ static TEE_Result csu_init(void)
 	const struct csu_config *csu_config = NULL;
 	const struct csu_setting *csu_setting = NULL;

+	if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))
+		IMSG("Device running with an insecure setting, consult the security reference manual from NXP");


May I suggest something like:
IMSG("Device running with an open security settings, please refer to OP-TEE i.MX platform documentation"); ?
OPTEE i.MX documentation being the readthedocs page where NXP will add the disclaimer.

Edit: In the end, I agree with @jforissier and @jenswi-linaro. I'm pretty sure other platforms feature equivalent mechanisms. Even optee core has some default insecure configuration (as mentionned for default_ta.pem). Would a global warning for all platforms make more sense?
This warning would redirect people to the documentation for additional information.

Emantor · 2021-11-18T15:40:52Z

core/arch/arm/plat-imx/conf.mk

+# correctly can be found at:
+#   - https://github.com/OP-TEE/optee_os/security/advisories/GHSA-6q85-3ph3-rm47
+#   - https://github.com/OP-TEE/optee_os/security/advisories/GHSA-4pqr-q8rf-8464
+CFG_IMX_INSECURE_CSU_SETTING ?= y


Please disable this for i.MX6UL, upstream OP-TEE configures the CSU correctly for this variant.

This is not correct, the i.MX6UL configuration is also incorrect per upcoming advisory.

This is not correct, the i.MX6UL configuration is also incorrect per upcoming advisory.

Please either post the info directly or point me to the advisory as soon as it becomes available.

Our advisory is now public here

jbech-linaro · 2021-11-19T07:58:34Z

@jenswi-linaro @jforissier @etienne-lms @clementfaure @Emantor , there was a lot of feedback in this PR. Let me try to summarize here:

Consensus seems to be that it's better to have a general warning enabled for our default configurations instead of having specific warnings spread out across the code base / platforms.
optee_docs is a better place to keep the information and we should mention that instead of vendor specific documentation.
A flag only enabling/disabling a print gives a false impression that a device is running in secure/insecure mode.

General warning

Where to place the print? __tee_entry_std()? That would spam a lot. call_initcalls() might be better?
How should we deal with devices that are behaving good (see @Emantor's comment above). I'm inclined to say that we always should have it enabled and leave it up to the end user to change the flag when building in their environment.
What should the print say? Probably easiest to just give a URL to the documentation. Something like
-- "OP-TEE running in insecure mode, see https://..." or
-- "OP-TEE running in unconfigured mode, see https://..."
Are we saying that we should close this PR and create a new one only meant for a global warning?
A global flag would be perfect, my only concern with a global flag is that it'll be too generic.

optee_docs and not the code

I've been suggesting both, Incident reporting: clarify the role of the OP-TEE project optee_docs#147 was a starting point for the docs, but just as an extra step I created this PR with the print. My intention was that if someone see that print, they'll look at the flag that enabled it and there we also have information about it.

Print gives false impression

Agree, hence why I added a long comment about it around the flag itself.
Initially I was thinking about checking against current CSU settings, but doing so is not trivial.
A global warning would definitely suffer from this as well.

etienne-lms · 2021-11-19T09:25:10Z

Thanks @jbech-linaro for the wrap-up 👍
Better a trace than nothing can still be a argument (despite what i've said). Having an IMSG() warning trace that is enabled by default looks legitimate to me.
E.g. CFG_UNSAFE_WARNING_BANNER ?= y
IMSG("This is a development OP-TEE: may expose sensitive data")
Disabling the config explicitly only disables a banner.

jforissier · 2021-11-19T09:28:14Z

@jbech-linaro here is my proposal.

Introduce CFG_WARN_INSECURE ?= y in mk/config.mk, which would print:

WARNING: This OP-TEE configuration might be insecure!
WARNING: Please check https://...

The message would conveniently be printed in core/arch/arm/kernel/boot.c immediately after the line:

OP-TEE version: xxx

Platforms which are known to be secure by default would simply set CFG_WARN_INSECURE ?= n in their plat-xxx/conf.mk to silence the warning.

clementfaure · 2021-11-19T11:26:28Z

HI @jbech-linaro ,
Thanks for the recap,
I would personally go with this proposal #4987 (comment)
It's a generic warning and flexible enough for the maintainers to define if their platforms/platform flavors should be considered as secure or insecure.
A warning message + a link to the documentation is okay to me.

jbech-linaro · 2021-11-19T12:34:59Z

@jbech-linaro here is my proposal.

Looking good Jerome. I'll rework and reuse this PR. Stay tuned.

jbech-linaro · 2021-11-22T16:03:30Z

@OP-TEE/maintainers , I've pushed another patch hopefully capturing the last set of comments. Please have a look.

b49020 · 2021-11-23T05:59:03Z

Looks reasonable to me:

Acked-by: Sumit Garg <sumit.garg@linaro.org>

Emantor · 2021-11-23T05:59:55Z

Acked-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>

ruchi393 · 2021-11-23T08:28:39Z

Reviewed-by: Ruchika Gupta <ruchika.gupta@linaro.org>

clementfaure · 2021-11-23T08:52:37Z

Acked-by: Clement Faure <clement.faure@nxp.com>

jbech-linaro · 2021-11-23T09:31:38Z

Thanks, I've added the R-B/A-B tags found so far, so the PR should be ready to merge. But if others would like to have the tag as well, please let me know and I'll add them as well.

jforissier · 2021-11-23T09:58:14Z

Acked-by: Jerome Forissier <jerome@forissier.org>

jbech-linaro · 2021-11-23T10:03:01Z

Acked-by: Jerome Forissier jerome@forissier.org

Added your tag as well, thanks!

jenswi-linaro · 2021-11-23T10:47:11Z

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

OP-TEE is a reference implementation for developers and device manufacturers, which implies that there always is a need to fill in missing pieces that cannot be done generically. The chipmakers often have additional security configurations those needs to be configured according to the chipmakers security guidelines and security specifications. To reduce the likelihood of running a vanilla configured OP-TEE we introduce the flag CFG_WARN_INSECURE that will give warning messages in the boot saying that the OP-TEE runs a configuration that might be insecure. The intention is that the device manufacturer making the end products should change the flag to "n" after implementing stubbed functionality in OP-TEE and configuring their device according to the chipmakers security guidelines and security specifications. Signed-off-by: Joakim Bech <joakim.bech@linaro.org> Reviewed-by: Ruchika Gupta <ruchika.gupta@linaro.org> Acked-by: Sumit Garg <sumit.garg@linaro.org> Acked-by: Rouven Czerwinski <r.czerwinski@pengutronix.de> Acked-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome@forissier.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

jbech-linaro · 2021-11-23T11:00:58Z

Acked-by: Jens Wiklander jens.wiklander@linaro.org

Tag added!

jforissier · 2021-11-23T13:45:49Z

@jbech-linaro thanks!

jbech-linaro commented Nov 18, 2021

View reviewed changes

jforissier approved these changes Nov 18, 2021

View reviewed changes

etienne-lms reviewed Nov 18, 2021

View reviewed changes

clementfaure reviewed Nov 18, 2021

View reviewed changes

Emantor reviewed Nov 18, 2021

View reviewed changes

jbech-linaro force-pushed the nxp-warning branch from 2b7adb1 to 3ae7c04 Compare November 22, 2021 16:02

jbech-linaro changed the title ~~plat-imx: highlight importance of configuring the CSU correctly~~ Introduce boot warning print for vanilla developer builds Nov 22, 2021

jbech-linaro force-pushed the nxp-warning branch from 3ae7c04 to f5fb244 Compare November 23, 2021 09:30

jbech-linaro force-pushed the nxp-warning branch from f5fb244 to df79392 Compare November 23, 2021 10:02

jbech-linaro force-pushed the nxp-warning branch from df79392 to f49f38d Compare November 23, 2021 11:00

jforissier merged commit 9e42008 into OP-TEE:master Nov 23, 2021

jbech-linaro deleted the nxp-warning branch November 23, 2021 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce boot warning print for vanilla developer builds #4987

Introduce boot warning print for vanilla developer builds #4987

jbech-linaro commented Nov 18, 2021

jbech-linaro Nov 18, 2021

clementfaure Nov 18, 2021 •

edited

Loading

jbech-linaro Nov 18, 2021

jforissier left a comment

jforissier Nov 18, 2021

etienne-lms Nov 18, 2021

etienne-lms Nov 18, 2021

etienne-lms Nov 18, 2021

jenswi-linaro commented Nov 18, 2021

clementfaure left a comment

clementfaure Nov 18, 2021

clementfaure Nov 18, 2021 •

edited

Loading

Emantor Nov 18, 2021

abarisani Nov 22, 2021

Emantor Nov 22, 2021

abarisani Nov 23, 2021 •

edited

Loading

jbech-linaro commented Nov 19, 2021

etienne-lms commented Nov 19, 2021

jforissier commented Nov 19, 2021

clementfaure commented Nov 19, 2021

jbech-linaro commented Nov 19, 2021

jbech-linaro commented Nov 22, 2021

b49020 commented Nov 23, 2021

Emantor commented Nov 23, 2021

ruchi393 commented Nov 23, 2021

clementfaure commented Nov 23, 2021

jbech-linaro commented Nov 23, 2021 •

edited

Loading

jforissier commented Nov 23, 2021

jbech-linaro commented Nov 23, 2021

jenswi-linaro commented Nov 23, 2021

jbech-linaro commented Nov 23, 2021

jforissier commented Nov 23, 2021

		if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))
		IMSG("Device running with an insecure setting, consult the security reference manual from NXP");

-	if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING))
-		IMSG("Device running with an insecure setting, consult the security reference manual from NXP");
+	 if (IS_ENABLED(CFG_IMX_INSECURE_CSU_SETTING)) {
+		IMSG("Device running with an insecure setting!");
+		IMSG("Consult the security reference manual from NXP");
+	}

Introduce boot warning print for vanilla developer builds #4987

Introduce boot warning print for vanilla developer builds #4987

Conversation

jbech-linaro commented Nov 18, 2021

Choose a reason for hiding this comment

clementfaure Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 18, 2021

clementfaure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementfaure Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarisani Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

jbech-linaro commented Nov 19, 2021

etienne-lms commented Nov 19, 2021

jforissier commented Nov 19, 2021

clementfaure commented Nov 19, 2021

jbech-linaro commented Nov 19, 2021

jbech-linaro commented Nov 22, 2021

b49020 commented Nov 23, 2021

Emantor commented Nov 23, 2021

ruchi393 commented Nov 23, 2021

clementfaure commented Nov 23, 2021

jbech-linaro commented Nov 23, 2021 • edited Loading

jforissier commented Nov 23, 2021

jbech-linaro commented Nov 23, 2021

jenswi-linaro commented Nov 23, 2021

jbech-linaro commented Nov 23, 2021

jforissier commented Nov 23, 2021

clementfaure Nov 18, 2021 •

edited

Loading

clementfaure Nov 18, 2021 •

edited

Loading

abarisani Nov 23, 2021 •

edited

Loading

jbech-linaro commented Nov 23, 2021 •

edited

Loading