Re: [PATCH v5 13/22] efi_loader: enable TEE runtime SetVariable() over FF-A

["Ilias Apalodimas" <ilias.apalodimas@linaro.org>]

Hi Harsimran


[...]

> +							   efi_uintn_t name_size,
> +							   const efi_guid_t *vendor,
> +							   struct var_check_property *var_property)
> +{
> +	struct smm_variable_var_check_property *smm_property;
> +	efi_uintn_t payload_size;
> +	u8 *comm_buf = NULL;
> +	efi_status_t ret;
> +
> +	payload_size = sizeof(*smm_property) + name_size;
> +	if (payload_size > max_payload_size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		return ret;
> +	}
> +	comm_buf = setup_mm_hdr((void **)&smm_property, payload_size,
> +				SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET,
> +				&ret);
> +	if (!comm_buf)
> +		return ret;
> +
> +	efi_memcpy_runtime(&smm_property->guid, vendor, sizeof(*vendor));
> +	smm_property->name_size = name_size;
> +	efi_memcpy_runtime(&smm_property->property, var_property,
> +			   sizeof(smm_property->property));
> +	efi_memcpy_runtime(smm_property->name, variable_name, name_size);
> +
> +	ret = mm_communicate(comm_buf, payload_size);
> +
> +	return ret;
> +}
> +
>  static efi_status_t get_property_int(const u16 *variable_name,
>  				     efi_uintn_t name_size,
>  				     const efi_guid_t *vendor,
> @@ -768,6 +800,49 @@ out:
>  	return ret;
>  }
>
> +static efi_status_t __efi_runtime get_property_int_runtime(const u16 *variable_name,
> +							   efi_uintn_t name_size,
> +							   const efi_guid_t *vendor,
> +							   struct var_check_property *var_property)
> +{

The changes between this and the non-runtime version are limited to a few memcpy/memset calls.
It's pretty easy to only keep the runtime version and get rid of all the duplication. Same
goes for the set_property_int()

> +	struct smm_variable_var_check_property *smm_property;
> +	efi_uintn_t payload_size;
> +	u8 *comm_buf = NULL;
> +	efi_status_t ret;
> +
> +	efi_memset_runtime(var_property, 0, sizeof(*var_property));
> +	payload_size = sizeof(*smm_property) + name_size;
> +	if (payload_size > max_payload_size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		return ret;
> +	}
> +	comm_buf = setup_mm_hdr((void **)&smm_property, payload_size,
> +				SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET,
> +				&ret);
> +	if (!comm_buf)
> +		return ret;
> +
> +	efi_memcpy_runtime(&smm_property->guid, vendor, sizeof(smm_property->guid));
> +	smm_property->name_size = name_size;
> +	efi_memcpy_runtime(smm_property->name, variable_name, name_size);
> +
> +	ret = mm_communicate(comm_buf, payload_size);
> +	/*
> +	 * Currently only R/O property is supported in StMM.
> +	 * Variables that are not set to R/O will not set the property in StMM
> +	 * and the call will return EFI_NOT_FOUND. We are setting the
> +	 * properties to 0x0 so checking against that is enough for the
> +	 * EFI_NOT_FOUND case.
> +	 */
> +	if (ret == EFI_NOT_FOUND)
> +		return EFI_SUCCESS;
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	efi_memcpy_runtime(var_property, &smm_property->property, sizeof(*var_property));
> +
> +	return EFI_SUCCESS;
> +}
> +
>  efi_status_t efi_get_variable_int(const u16 *variable_name,
>  				  const efi_guid_t *vendor,
>  				  u32 *attributes, efi_uintn_t *data_size,
> @@ -1001,7 +1076,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>  		var_property.property |= VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY;
>  		var_property.attributes = attributes;
>  		var_property.minsize = 1;
> -		var_property.maxsize = var_acc->data_size;
> +		var_property.maxsize = data_size;
>  		ret = set_property_int(variable_name, name_size, vendor, &var_property);
>  	}
>
> @@ -1078,7 +1153,7 @@ efi_query_variable_info_runtime(u32 attributes, u64 *max_variable_storage_size,
>  }
>
>  /**
> - * efi_set_variable_runtime() - runtime implementation of SetVariable()
> + * efi_set_variable_int_runtime() - runtime implementation of SetVariable()
>   *
>   * @variable_name:	name of the variable
>   * @guid:		vendor GUID
> @@ -1088,11 +1163,69 @@ efi_query_variable_info_runtime(u32 attributes, u64 *max_variable_storage_size,
>   * Return:		status code
>   */
>  static efi_status_t __efi_runtime EFIAPI
> -efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> -			 u32 attributes, efi_uintn_t data_size,
> -			 const void *data)
> +efi_set_variable_int_runtime(u16 *variable_name, const efi_guid_t *guid,
> +			     u32 attributes, efi_uintn_t data_size,
> +			     const void *data)
>  {
> -	return EFI_UNSUPPORTED;
> +	efi_status_t ret, mm_communicate_ret = EFI_SUCCESS;
> +	struct var_check_property var_property;
> +	struct smm_variable_access *var_acc;
> +	efi_uintn_t payload_size;
> +	efi_uintn_t name_size;
> +	u8 *comm_buf = NULL;
> +	bool ro;
> +
> +	if (!variable_name || variable_name[0] == 0 || !guid)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (data_size > 0 && !data)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/* Check payload size */
> +	name_size = u16_strsize(variable_name);
> +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> +	if (payload_size > max_payload_size)
> +		return EFI_INVALID_PARAMETER;
> +
> +	ro = !!(attributes & EFI_VARIABLE_READ_ONLY);
> +	attributes &= EFI_VARIABLE_MASK;
> +
> +	ret = get_property_int_runtime(variable_name, name_size, guid,
> +				       &var_property);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	if (var_property.property & VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY)
> +		return EFI_WRITE_PROTECTED;
> +
> +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> +				SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> +	if (!comm_buf)
> +		return ret;
> +
> +	/* Fill in contents */
> +	efi_memcpy_runtime(&var_acc->guid, guid, sizeof(*guid));
> +	var_acc->data_size = data_size;
> +	var_acc->name_size = name_size;
> +	var_acc->attr = attributes;
> +	efi_memcpy_runtime(var_acc->name, variable_name, name_size);
> +	efi_memcpy_runtime((u8 *)var_acc->name + name_size, data, data_size);
> +
> +	/* Communicate */
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret != EFI_SUCCESS)
> +		mm_communicate_ret = ret;
> +
> +	if (ro && !(var_property.property & VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY)) {
> +		var_property.revision = VAR_CHECK_VARIABLE_PROPERTY_REVISION;
> +		var_property.property |= VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY;
> +		var_property.attributes = attributes;
> +		var_property.minsize = 1;
> +		var_property.maxsize = data_size;
> +		ret = set_property_int_runtime(variable_name, name_size, guid, &var_property);
> +	}
> +
> +	return (mm_communicate_ret == EFI_SUCCESS) ? ret : mm_communicate_ret;
>  }
>
>  /**
> @@ -1129,7 +1262,7 @@ void efi_variables_boot_exit_notify(void)
>  	efi_runtime_services.get_variable = efi_get_variable_runtime;
>  	efi_runtime_services.get_next_variable_name =
>  			efi_get_next_variable_name_runtime;


All the get_variable*() calls used to point to the function from efi_var_common.c.
By changing this you are breaking the runtime case for OP-TEE since now we will get
EFI_OUT_OF_RESOURCES.
There's no easy way to fix this at runtime. For OP-TEE the kernel might replace this call
with an internal OP-TEE call if it's part of the firmware and announces it can provide
EFI runtime services. However, the default uuse case, which we don't support SetVariable
at runtime only needs to keep working.

I'll post a script next week so you can replicate and test this in QEMU with OP-TEE etc.

Cheers
/Ilias

> -	efi_runtime_services.set_variable = efi_set_variable_runtime;
> +	efi_runtime_services.set_variable = efi_set_variable_int_runtime;
>  	efi_update_table_header_crc32(&efi_runtime_services.hdr);
>
>  	/* Record that ExitBootServices() has been called */