Re: [PATCH 1/2] habv4: correct habv4 rom vector table

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Stefan,

On 11.01.24 16:42, Stefan Kerkmann wrote:
> All function signatures have been taken from the NXP manual "High
> Assurance Boot Version 4 Application Programming Interface Reference
> Manual" revision 1.4 under section "4.5 ROM vector table". A copy can be
> obtained from the imx code signing tool (imx-cst).
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>  drivers/hab/habv4.c | 50 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index ed6d4db77c..dfa0207435 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -144,31 +144,41 @@ struct hab_header {
>  	uint8_t par;
>  } __packed;
>  
> -typedef enum hab_status hab_loader_callback_fn(void **start, uint32_t *bytes, const void *boot_data);
> +typedef enum hab_status hab_loader_callback_fn(void **start, size_t *bytes, const void *boot_data);
> +typedef void (*hab_image_entry_fn)(void);

While it only matter for forward declaration, perhaps, change this
to be a non-pointer like hab_loader_callback_fn above?

>  
> +/* This table is constructed from the NXP manual "High Assurance Boot Version 4
> + * Application Programming Interface Reference Manual", section 4.5 ROM vector
> + * table. Revision 1.4 */
>  struct habv4_rvt {
>  	struct hab_header header;
>  	enum hab_status (*entry)(void);
>  	enum hab_status (*exit)(void);
> -	enum hab_status (*check_target)(enum hab_target target, const void *start, uint32_t bytes);
> -	void *(*authenticate_image)(uint8_t cid, uint32_t ivt_offset, void **start, uint32_t *bytes, hab_loader_callback_fn *loader);
> -	enum hab_status (*run_dcd)(const void *dcd);
> -	enum hab_status (*run_csf)(const void *csf, uint8_t cid);
> +	enum hab_status (*check_target)(enum hab_target target, const void *start, size_t bytes);
> +	void *(*authenticate_image)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn *loader);

Here you explicitly use a pointer to a function.

> +	enum hab_status (*run_dcd)(const uint8_t *dcd);
> +	enum hab_status (*run_csf)(const uint8_t *csf, uint8_t cid, uint32_t srkmask);
>  	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_event)(enum hab_status status, uint32_t index, uint8_t *event, size_t *bytes);
>  	enum hab_status (*report_status)(enum hab_config *config, enum habv4_state *state);
>  	void (*failsafe)(void);
> +	hab_image_entry_fn(* authenticate_image_no_dcd)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn loader);

And here you rely on a function pointer being automatically derived.
While that's ok from a correctness point of view, for symmetry, it
would be better to stick to one type.

> +	uint32_t(* get_version)(void);

Nitpick: space after uint32_t and not before get_version.

> +	enum hab_status (*authenticate_container)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn loader, uint32_t srkmask, int skip_dcd);
>  } __packed;
>  
> -#define FSL_SIP_HAB             0xC2000007

This is removed without replacement?

> -#define FSL_SIP_HAB_AUTHENTICATE        0x00
> -#define FSL_SIP_HAB_ENTRY               0x01
> -#define FSL_SIP_HAB_EXIT                0x02
> -#define FSL_SIP_HAB_REPORT_EVENT        0x03
> -#define FSL_SIP_HAB_REPORT_STATUS       0x04
> -#define FSL_SIP_HAB_FAILSAFE            0x05
> -#define FSL_SIP_HAB_CHECK_TARGET        0x06
> -#define FSL_SIP_HAB_GET_VERSION		0x07
> +#define FSL_SIP_HAB 0xC2000007
> +
> +enum hab_sip_cmd {
> +	FSL_SIP_HAB_AUTHENTICATE = 0x00,
> +	FSL_SIP_HAB_ENTRY = 0x01,
> +	FSL_SIP_HAB_EXIT = 0x02,
> +	FSL_SIP_HAB_REPORT_EVENT = 0x03,
> +	FSL_SIP_HAB_REPORT_STATUS = 0x04,
> +	FSL_SIP_HAB_FAILSAFE = 0x05,
> +	FSL_SIP_HAB_CHECK_TARGET = 0x06,
> +	FSL_SIP_HAB_GET_VERSION = 0x07,
> +};
>  
>  static enum hab_status hab_sip_report_status(enum hab_config *config,
>  					     enum habv4_state *state)
> @@ -211,8 +221,8 @@ static uint32_t hab_sip_get_version(void)
>  #define IMX8MP_ROM_OCRAM_ADDRESS	0x90D040
>  
>  static enum hab_status imx8m_read_sram_events(enum hab_status status,
> -					     uint32_t index, void *event,
> -					     uint32_t *bytes)
> +					     uint32_t index, uint8_t *event,
> +					     size_t *bytes)
>  {
>  	struct hab_event_record *events[10];
>  	int num_events = 0;
> @@ -478,7 +488,7 @@ static void habv4_display_event_record(struct hab_event_record *record)
>  	pr_err("Engine: %s (0x%02x)\n", habv4_get_engine_str(record->engine), record->engine);
>  }
>  
> -static void habv4_display_event(uint8_t *data, uint32_t len)
> +static void habv4_display_event(uint8_t *data, size_t len)
>  {
>  	unsigned int i;
>  
> @@ -525,7 +535,7 @@ static bool is_known_rng_fail_event(const uint8_t *data, size_t len)
>  	return false;
>  }
>  
> -static uint8_t *hab_get_event(const struct habv4_rvt *rvt, int index, int *len)
> +static uint8_t *hab_get_event(const struct habv4_rvt *rvt, int index, size_t *len)
>  {
>  	enum hab_status err;
>  	uint8_t *buf;
> @@ -558,7 +568,7 @@ int habv4_get_state(void)
>  static int habv4_get_status(const struct habv4_rvt *rvt)
>  {
>  	uint8_t *data;
> -	uint32_t len;
> +	size_t len;
>  	int i;
>  	enum hab_status status;
>  	enum hab_config config = 0x0;
> 

-- 
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 |