[PATCH 1/4] commands: hab: check for error in imx_hab_device_locked_down

[PATCH 1/4] commands: hab: check for error in imx_hab_device_locked_down

[Ahmad Fatoum]

imx_hab_device_locked_down() reads efuses and that operation can fail.
Instead of assuming a failure means the efuses are non-zero, have the
hab command explicitly check for negative error codes.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/hab.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/commands/hab.c b/commands/hab.c
index 97a1701fa551..8ae943a4c880 100644
--- a/commands/hab.c
+++ b/commands/hab.c
@@ -58,7 +58,13 @@ static int do_hab(int argc, char *argv[])
 			printf("%02x", srk[i]);
 		printf("\n");
 
-		if (imx_hab_device_locked_down())
+		ret = imx_hab_device_locked_down();
+		if (ret < 0) {
+			printf("failed to determine lockdown mode: '%pe'\n", ERR_PTR(ret));
+			return ret;
+		}
+
+		if (ret)
 			printf("secure mode\n");
 		else
 			printf("devel mode\n");
-- 
2.39.2

[PATCH 2/4] HAB: guard against NULL imx_hab_ops in imx_hab_device_locked_down()

[Ahmad Fatoum]

All other exported functions guard against imx_get_hab_ops() returning
NULL, before dereferencing the returned pointer. Do likewise in
imx_hab_device_locked_down().

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/hab/hab.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hab/hab.c b/drivers/hab/hab.c
index 5bb97c4b689b..2a2d347dd68f 100644
--- a/drivers/hab/hab.c
+++ b/drivers/hab/hab.c
@@ -351,5 +351,8 @@ int imx_hab_device_locked_down(void)
 {
 	struct imx_hab_ops *ops = imx_get_hab_ops();
 
+	if (!ops)
+		return -ENOSYS;
+
 	return ops->device_locked_down();
 }
-- 
2.39.2

[PATCH 3/4] nvmem: ocotp: handle too early calls into ocotp driver gracefully

[Ahmad Fatoum]

HAB code calls into OCOTP driver by relying on a global imx_ocotp
variable that's populated on driver probe.

For board code that calls a HAB function to early, this may end up
dereferencing a NULL pointer, so let's return -EPROBE_DEFER in that
case or if deep probe is enabled, just probe the OCOTP directly.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-vf610.c |  5 ++++-
 drivers/nvmem/ocotp.c       | 41 +++++++++++++++++++++++++++++++++++--
 include/mach/imx/ocotp.h    |  2 +-
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/imx/clk-vf610.c b/drivers/clk/imx/clk-vf610.c
index 89899e0dc9b4..112f64df9b74 100644
--- a/drivers/clk/imx/clk-vf610.c
+++ b/drivers/clk/imx/clk-vf610.c
@@ -568,13 +568,16 @@ static int vf610_switch_cpu_clock_to_400mhz(void)
 static int vf610_switch_cpu_clock(void)
 {
 	int ret;
-	bool sense_enable;
+	int sense_enable;
 	uint32_t speed_grading;
 
 	if (!of_machine_is_compatible("fsl,vf610"))
 		return 0;
 
 	sense_enable = imx_ocotp_sense_enable(true);
+	if (sense_enable < 0)
+		return sense_enable;
+
 	ret = imx_ocotp_read_field(VF610_OCOTP_SPEED_GRADING, &speed_grading);
 	imx_ocotp_sense_enable(sense_enable);
 	if (ret < 0)
diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
index 8ba7a8af5da5..c22e5d9585fa 100644
--- a/drivers/nvmem/ocotp.c
+++ b/drivers/nvmem/ocotp.c
@@ -14,6 +14,7 @@
  */
 
 #include <common.h>
+#include <deep-probe.h>
 #include <driver.h>
 #include <malloc.h>
 #include <xfuncs.h>
@@ -497,11 +498,17 @@ static void imx_ocotp_field_decode(uint32_t field, unsigned *word,
 	*mask = GENMASK(width, 0);
 }
 
+static int imx_ocotp_ensure_probed(void);
+
 int imx_ocotp_read_field(uint32_t field, unsigned *value)
 {
 	unsigned word, bit, mask, val;
 	int ret;
 
+	ret = imx_ocotp_ensure_probed();
+	if (ret)
+		return ret;
+
 	imx_ocotp_field_decode(field, &word, &bit, &mask);
 
 	ret = imx_ocotp_reg_read(imx_ocotp, word, &val);
@@ -524,6 +531,10 @@ int imx_ocotp_write_field(uint32_t field, unsigned value)
 	unsigned word, bit, mask;
 	int ret;
 
+	ret = imx_ocotp_ensure_probed();
+	if (ret)
+		return ret;
+
 	imx_ocotp_field_decode(field, &word, &bit, &mask);
 
 	value &= mask;
@@ -541,14 +552,27 @@ int imx_ocotp_write_field(uint32_t field, unsigned value)
 
 int imx_ocotp_permanent_write(int enable)
 {
+	int ret;
+
+	ret = imx_ocotp_ensure_probed();
+	if (ret)
+		return ret;
+
 	imx_ocotp->permanent_write_enable = enable;
 
 	return 0;
 }
 
-bool imx_ocotp_sense_enable(bool enable)
+int imx_ocotp_sense_enable(bool enable)
 {
-	const bool old_value = imx_ocotp->sense_enable;
+	bool old_value;
+	int ret;
+
+	ret = imx_ocotp_ensure_probed();
+	if (ret)
+		return ret;
+
+	old_value = imx_ocotp->sense_enable;
 	imx_ocotp->sense_enable = enable;
 	return old_value;
 }
@@ -994,6 +1018,19 @@ static __maybe_unused struct of_device_id imx_ocotp_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, imx_ocotp_dt_ids);
 
+static int imx_ocotp_ensure_probed(void)
+{
+	if (!imx_ocotp && deep_probe_is_supported()) {
+		int ret;
+
+		ret = of_devices_ensure_probed_by_dev_id(imx_ocotp_dt_ids);
+		if (ret)
+			return ret;
+	}
+
+	return imx_ocotp ? 0 : -EPROBE_DEFER;
+}
+
 static struct driver imx_ocotp_driver = {
 	.name	= "imx_ocotp",
 	.probe	= imx_ocotp_probe,
diff --git a/include/mach/imx/ocotp.h b/include/mach/imx/ocotp.h
index 7c72edfb22a7..5f7b88f716a7 100644
--- a/include/mach/imx/ocotp.h
+++ b/include/mach/imx/ocotp.h
@@ -35,7 +35,7 @@
 int imx_ocotp_read_field(uint32_t field, unsigned *value);
 int imx_ocotp_write_field(uint32_t field, unsigned value);
 int imx_ocotp_permanent_write(int enable);
-bool imx_ocotp_sense_enable(bool enable);
+int imx_ocotp_sense_enable(bool enable);
 
 static inline u64 imx_ocotp_read_uid(void __iomem *ocotp)
 {
-- 
2.39.2

Re: [PATCH 3/4] nvmem: ocotp: handle too early calls into ocotp driver gracefully

[Marco Felsch]

On 23-07-26, Ahmad Fatoum wrote:
> HAB code calls into OCOTP driver by relying on a global imx_ocotp
> variable that's populated on driver probe.
> 
> For board code that calls a HAB function to early, this may end up
> dereferencing a NULL pointer, so let's return -EPROBE_DEFER in that
> case or if deep probe is enabled, just probe the OCOTP directly.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/clk/imx/clk-vf610.c |  5 ++++-
>  drivers/nvmem/ocotp.c       | 41 +++++++++++++++++++++++++++++++++++--
>  include/mach/imx/ocotp.h    |  2 +-
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-vf610.c b/drivers/clk/imx/clk-vf610.c
> index 89899e0dc9b4..112f64df9b74 100644
> --- a/drivers/clk/imx/clk-vf610.c
> +++ b/drivers/clk/imx/clk-vf610.c
> @@ -568,13 +568,16 @@ static int vf610_switch_cpu_clock_to_400mhz(void)
>  static int vf610_switch_cpu_clock(void)
>  {
>  	int ret;
> -	bool sense_enable;
> +	int sense_enable;
>  	uint32_t speed_grading;
>  
>  	if (!of_machine_is_compatible("fsl,vf610"))
>  		return 0;
>  
>  	sense_enable = imx_ocotp_sense_enable(true);
> +	if (sense_enable < 0)
> +		return sense_enable;
> +
>  	ret = imx_ocotp_read_field(VF610_OCOTP_SPEED_GRADING, &speed_grading);
>  	imx_ocotp_sense_enable(sense_enable);
>  	if (ret < 0)
> diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
> index 8ba7a8af5da5..c22e5d9585fa 100644
> --- a/drivers/nvmem/ocotp.c
> +++ b/drivers/nvmem/ocotp.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include <common.h>
> +#include <deep-probe.h>
>  #include <driver.h>
>  #include <malloc.h>
>  #include <xfuncs.h>
> @@ -497,11 +498,17 @@ static void imx_ocotp_field_decode(uint32_t field, unsigned *word,
>  	*mask = GENMASK(width, 0);
>  }
>  
> +static int imx_ocotp_ensure_probed(void);

Nit: Move the function definition here?

Regards,
  Marco

>  int imx_ocotp_read_field(uint32_t field, unsigned *value)
>  {
>  	unsigned word, bit, mask, val;
>  	int ret;
>  
> +	ret = imx_ocotp_ensure_probed();
> +	if (ret)
> +		return ret;
> +
>  	imx_ocotp_field_decode(field, &word, &bit, &mask);
>  
>  	ret = imx_ocotp_reg_read(imx_ocotp, word, &val);
> @@ -524,6 +531,10 @@ int imx_ocotp_write_field(uint32_t field, unsigned value)
>  	unsigned word, bit, mask;
>  	int ret;
>  
> +	ret = imx_ocotp_ensure_probed();
> +	if (ret)
> +		return ret;
> +
>  	imx_ocotp_field_decode(field, &word, &bit, &mask);
>  
>  	value &= mask;
> @@ -541,14 +552,27 @@ int imx_ocotp_write_field(uint32_t field, unsigned value)
>  
>  int imx_ocotp_permanent_write(int enable)
>  {
> +	int ret;
> +
> +	ret = imx_ocotp_ensure_probed();
> +	if (ret)
> +		return ret;
> +
>  	imx_ocotp->permanent_write_enable = enable;
>  
>  	return 0;
>  }
>  
> -bool imx_ocotp_sense_enable(bool enable)
> +int imx_ocotp_sense_enable(bool enable)
>  {
> -	const bool old_value = imx_ocotp->sense_enable;
> +	bool old_value;
> +	int ret;
> +
> +	ret = imx_ocotp_ensure_probed();
> +	if (ret)
> +		return ret;
> +
> +	old_value = imx_ocotp->sense_enable;
>  	imx_ocotp->sense_enable = enable;
>  	return old_value;
>  }
> @@ -994,6 +1018,19 @@ static __maybe_unused struct of_device_id imx_ocotp_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, imx_ocotp_dt_ids);
>  
> +static int imx_ocotp_ensure_probed(void)
> +{
> +	if (!imx_ocotp && deep_probe_is_supported()) {
> +		int ret;
> +
> +		ret = of_devices_ensure_probed_by_dev_id(imx_ocotp_dt_ids);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return imx_ocotp ? 0 : -EPROBE_DEFER;
> +}
> +
>  static struct driver imx_ocotp_driver = {
>  	.name	= "imx_ocotp",
>  	.probe	= imx_ocotp_probe,
> diff --git a/include/mach/imx/ocotp.h b/include/mach/imx/ocotp.h
> index 7c72edfb22a7..5f7b88f716a7 100644
> --- a/include/mach/imx/ocotp.h
> +++ b/include/mach/imx/ocotp.h
> @@ -35,7 +35,7 @@
>  int imx_ocotp_read_field(uint32_t field, unsigned *value);
>  int imx_ocotp_write_field(uint32_t field, unsigned value);
>  int imx_ocotp_permanent_write(int enable);
> -bool imx_ocotp_sense_enable(bool enable);
> +int imx_ocotp_sense_enable(bool enable);
>  
>  static inline u64 imx_ocotp_read_uid(void __iomem *ocotp)
>  {
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 3/4] nvmem: ocotp: handle too early calls into ocotp driver gracefully

[Ahmad Fatoum]

On 27.07.23 08:05, Marco Felsch wrote:
> On 23-07-26, Ahmad Fatoum wrote:
>> HAB code calls into OCOTP driver by relying on a global imx_ocotp
>> variable that's populated on driver probe.
>>
>> For board code that calls a HAB function to early, this may end up
>> dereferencing a NULL pointer, so let's return -EPROBE_DEFER in that
>> case or if deep probe is enabled, just probe the OCOTP directly.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---

>> @@ -497,11 +498,17 @@ static void imx_ocotp_field_decode(uint32_t field, unsigned *word,
>>  	*mask = GENMASK(width, 0);
>>  }
>>  
>> +static int imx_ocotp_ensure_probed(void);
> 
> Nit: Move the function definition here?

Then I'd have to forward-declare imx_ocotp_dt_ids.
I choose to keep the probing stuff together.


>> +static int imx_ocotp_ensure_probed(void)
>> +{
>> +	if (!imx_ocotp && deep_probe_is_supported()) {
>> +		int ret;
>> +
>> +		ret = of_devices_ensure_probed_by_dev_id(imx_ocotp_dt_ids);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return imx_ocotp ? 0 : -EPROBE_DEFER;
>> +}
>> +

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 4/4] hab: habv4: export function to query HAB state

[Ahmad Fatoum]

Board code may want to base runtime decisions on whether the system
is secure booting. Add a function to query that state.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/hab/habv4.c | 29 +++++++++++++----------------
 include/hab.h       | 17 +++++++++++++++++
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index b6baa92c679d..9f54aed5f508 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -68,18 +68,6 @@ enum hab_config {
 	HAB_CONFIG_CLOSED = 0xcc,	/* Secure IC */
 };
 
-/* State definitions */
-enum hab_state {
-	HAB_STATE_INITIAL = 0x33,	/* Initialising state (transitory) */
-	HAB_STATE_CHECK = 0x55,		/* Check state (non-secure) */
-	HAB_STATE_NONSECURE = 0x66,	/* Non-secure state */
-	HAB_STATE_TRUSTED = 0x99,	/* Trusted state */
-	HAB_STATE_SECURE = 0xaa,	/* Secure state */
-	HAB_STATE_FAIL_SOFT = 0xcc,	/* Soft fail state */
-	HAB_STATE_FAIL_HARD = 0xff,	/* Hard fail state (terminal) */
-	HAB_STATE_NONE = 0xf0,		/* No security state machine */
-};
-
 enum hab_reason {
 	HAB_REASON_RSN_ANY = 0x00,		/* Match any reason */
 	HAB_REASON_UNS_COMMAND = 0x03,		/* Unsupported command */
@@ -168,7 +156,7 @@ struct habv4_rvt {
 	enum hab_status (*run_csf)(const void *csf, uint8_t cid);
 	enum hab_status (*assert)(enum hab_assertion assertion, const void *data, uint32_t count);
 	enum hab_status (*report_event)(enum hab_status status, uint32_t index, void *event, uint32_t *bytes);
-	enum hab_status (*report_status)(enum hab_config *config, enum hab_state *state);
+	enum hab_status (*report_status)(enum hab_config *config, enum habv4_state *state);
 	void (*failsafe)(void);
 } __packed;
 
@@ -182,7 +170,7 @@ struct habv4_rvt {
 #define FSL_SIP_HAB_CHECK_TARGET        0x06
 
 static enum hab_status hab_sip_report_status(enum hab_config *config,
-					     enum hab_state *state)
+					     enum habv4_state *state)
 {
 	struct arm_smccc_res res;
 
@@ -290,7 +278,7 @@ static const char *habv4_get_config_str(enum hab_config config)
 	return "<unknown>";
 }
 
-static const char *habv4_get_state_str(enum hab_state state)
+static const char *habv4_get_state_str(enum habv4_state state)
 {
 	switch (state) {
 	case HAB_STATE_INITIAL:
@@ -518,6 +506,13 @@ static uint8_t *hab_get_event(const struct habv4_rvt *rvt, int index, int *len)
 	return buf;
 }
 
+static int habv4_state = -EPROBE_DEFER;
+
+int habv4_get_state(void)
+{
+	return habv4_state;
+}
+
 static int habv4_get_status(const struct habv4_rvt *rvt)
 {
 	uint8_t *data;
@@ -525,7 +520,7 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 	int i;
 	enum hab_status status;
 	enum hab_config config = 0x0;
-	enum hab_state state = 0x0;
+	enum habv4_state state = 0x0;
 
 	if (rvt->header.tag != HAB_TAG_RVT) {
 		pr_err("ERROR - RVT not found!\n");
@@ -533,6 +528,8 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 	}
 
 	status = rvt->report_status(&config, &state);
+	habv4_state = state;
+
 	pr_info("Status: %s (0x%02x)\n", habv4_get_status_str(status), status);
 	pr_info("Config: %s (0x%02x)\n", habv4_get_config_str(config), config);
 	pr_info("State: %s (0x%02x)\n",	habv4_get_state_str(state), state);
diff --git a/include/hab.h b/include/hab.h
index d594ad9ee185..ebe19ce357a6 100644
--- a/include/hab.h
+++ b/include/hab.h
@@ -8,9 +8,22 @@
 
 #include <errno.h>
 
+/* State definitions */
+enum habv4_state {
+	HAB_STATE_INITIAL = 0x33,	/* Initialising state (transitory) */
+	HAB_STATE_CHECK = 0x55,		/* Check state (non-secure) */
+	HAB_STATE_NONSECURE = 0x66,	/* Non-secure state */
+	HAB_STATE_TRUSTED = 0x99,	/* Trusted state */
+	HAB_STATE_SECURE = 0xaa,	/* Secure state */
+	HAB_STATE_FAIL_SOFT = 0xcc,	/* Soft fail state */
+	HAB_STATE_FAIL_HARD = 0xff,	/* Hard fail state (terminal) */
+	HAB_STATE_NONE = 0xf0,		/* No security state machine */
+};
+
 #ifdef CONFIG_HABV4
 int imx28_hab_get_status(void);
 int imx6_hab_get_status(void);
+int habv4_get_state(void);
 #else
 static inline int imx28_hab_get_status(void)
 {
@@ -20,6 +33,10 @@ static inline int imx6_hab_get_status(void)
 {
 	return -EPERM;
 }
+static inline int habv4_get_state(void)
+{
+	return -ENOSYS;
+}
 #endif
 
 #ifdef CONFIG_HABV3
-- 
2.39.2

Re: [PATCH 1/4] commands: hab: check for error in imx_hab_device_locked_down

[Marco Felsch]

On 23-07-26, Ahmad Fatoum wrote:
> imx_hab_device_locked_down() reads efuses and that operation can fail.
> Instead of assuming a failure means the efuses are non-zero, have the
> hab command explicitly check for negative error codes.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Feel free to add my r-b for the whole series.

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH 1/4] commands: hab: check for error in imx_hab_device_locked_down

[Sascha Hauer]

On Wed, Jul 26, 2023 at 09:27:15PM +0200, Ahmad Fatoum wrote:
> imx_hab_device_locked_down() reads efuses and that operation can fail.
> Instead of assuming a failure means the efuses are non-zero, have the
> hab command explicitly check for negative error codes.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  commands/hab.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Applied, thanks

Sascha

> 
> diff --git a/commands/hab.c b/commands/hab.c
> index 97a1701fa551..8ae943a4c880 100644
> --- a/commands/hab.c
> +++ b/commands/hab.c
> @@ -58,7 +58,13 @@ static int do_hab(int argc, char *argv[])
>  			printf("%02x", srk[i]);
>  		printf("\n");
>  
> -		if (imx_hab_device_locked_down())
> +		ret = imx_hab_device_locked_down();
> +		if (ret < 0) {
> +			printf("failed to determine lockdown mode: '%pe'\n", ERR_PTR(ret));
> +			return ret;
> +		}
> +
> +		if (ret)
>  			printf("secure mode\n");
>  		else
>  			printf("devel mode\n");
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |