[RESEND,RFC,03/10] FWU: Add metadata structure and functions for accessing metadata

In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, which is stored on
a dedicated partition. Add the metadata structure, and functions to
access the metadata. These are generic API's, and implementations can
be added based on parameters like how the metadata partition is
accessed and what type of storage device houses the metadata.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/fwu_metadata.h         | 125 +++++++++++++++
 lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
 2 files changed, 400 insertions(+)
 create mode 100644 include/fwu_metadata.h
 create mode 100644 lib/fwu_updates/fwu_metadata.c

Hi Sughosh, 

On Thu, Nov 25, 2021 at 12:42:55PM +0530, Sughosh Ganu wrote:
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, which is stored on
> a dedicated partition. Add the metadata structure, and functions to
> access the metadata. These are generic API's, and implementations can
> be added based on parameters like how the metadata partition is
> accessed and what type of storage device houses the metadata.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/fwu_metadata.h         | 125 +++++++++++++++
>  lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
>  2 files changed, 400 insertions(+)
>  create mode 100644 include/fwu_metadata.h
>  create mode 100644 lib/fwu_updates/fwu_metadata.c
> 
> diff --git a/include/fwu_metadata.h b/include/fwu_metadata.h
> new file mode 100644
> index 0000000000..e692ef7506
> --- /dev/null
> +++ b/include/fwu_metadata.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2021, Linaro Limited

Please add a link here to the arm spec that describes the metadata etc

> + */
> +
> +#if !defined _FWU_METADATA_H_
> +#define _FWU_METADATA_H_
> +
> +#include <blk.h>
> +#include <efi.h>
> +#include <uuid.h>
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct fwu_image_bank_info - firmware image information
> + * @image_uuid: Guid value of the image in this bank
> + * @accepted: Acceptance status of the image
> + * @reserved: Reserved
> + *
> + * The structure contains image specific fields which are
> + * used to identify the image and to specify the image's
> + * acceptance status
> + */
> +struct fwu_image_bank_info {
> +	efi_guid_t  image_uuid;
> +	u32 accepted;
> +	u32 reserved;
> +};

fwu_image_bank_info -> fwu_img_bank_info

> +
> +/**
> + * struct fwu_image_entry - information for a particular type of image
> + * @image_type_uuid: Guid value for identifying the image type
> + * @location_uuid: Guid of the storage volume where the image is located

/s/Guid/GUID

> + * @img_bank_info: Array containing properties of images
> + *
> + * This structure contains information on various types of updatable
> + * firmware images. Each image type then contains an array of image
> + * information per bank.
> + */
> +struct fwu_image_entry {
> +	efi_guid_t image_type_uuid;
> +	efi_guid_t location_uuid;
> +	struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> +};
> +

It seems like you've followed the naming proposed in the spec,  which makes
reading spec -- code easier.  However I feel we should add a few more
comments on the naming to make reading easier or change the naming and
mention the original name in comments.

A 'bank' is supposed to contain:
bank[0]: Uboot(0), TF-A(0) etc
bank[1]: Uboot(1), TF-A(1) etc
However there's no structure that defines an entire bank.  Instead the bank
information is constructed by reading the metadata and fixing it up on
the fly.

fwu_image_bank_info -- Information for a specific image (e.g OP-TEE,
U-Boot, TF-A, whatever) but not within a *bank*.  That's amongst a
collection of images of the same type.

IOW img_bank_info looks like:
img_bank_info[0] -> U-Boot(0), U-Boot(1) etc
img_bank_info[1] -> TF-A(0), TF-A(1) etc

@Jose can we tweak the spec naming a bit to be more intuitive?
I am terrible at naming stuff but what about:
fwu_image_bank_info -> fwu_img_repo_info, fwu_img_vault_info, 
					   fwu_img_storage_info, fwu_img_array_info, 


> +/**
> + * struct fwu_metadata - Metadata structure for multi-bank updates
> + * @crc32: crc32 value for the metadata
> + * @version: Metadata version
> + * @active_index: Index of the bank currently used for booting images
> + * @previous_active_inde: Index of the bank used before the current bank
> + *                        being used for booting
> + * @img_entry: Array of information on various firmware images that can
> + *             be updated
> + *
> + * This structure is used to store all the needed information for performing
> + * multi bank updates on the platform. This contains info on the bank being
> + * used to boot along with the information needed for identification of
> + * individual images
> + */
> +struct fwu_metadata {
> +	u32 crc32;
> +	u32 version;
> +	u32 active_index;
> +	u32 previous_active_index;
> +
> +	struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> +};
> +
> +/**
> + * @get_active_index: get the current active_index value
> + * @update_active_index: update the active_index value
> + * @fill_partition_guid_array: fill the array with guid values of the
> + *                             partitions found on the storage media
> + * @get_image_alt_num: get the alt number to be used for the image
> + * @metadata_check: check the validity of the metadata partitions
> + * @revert_boot_index: set the active_index to previous_active_index
> + * @set_accept_image: set the accepted bit for the image
> + * @clear_accept_image: clear the accepted bit for the image
> + * @get_metadata() - Get a metadata copy
> + */
> +struct fwu_metadata_ops {
> +	int (*get_active_index)(u32 *active_idx);
> +
> +	int (*update_active_index)(u32 active_idx);
> +
> +	int (*fill_partition_guid_array)(efi_guid_t **part_guid_arr,
> +					 u32 *nparts);
> +
> +	int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> +				 int *alt_num);
> +
> +	int (*metadata_check)(void);
> +
> +	int (*revert_boot_index)(u32 *active_idx);
> +
> +	int (*set_accept_image)(efi_guid_t *img_type_id);
> +
> +	int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> +
> +	int (*get_metadata)(struct fwu_metadata **metadata);
> +};
> +
> +#define FWU_METADATA_GUID \
> +	EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> +		 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> +
> +#define FWU_METADATA_VERSION	0x1
> +
> +extern struct fwu_metadata_ops fwu_gpt_blk_ops;
> +
> +struct fwu_metadata_ops *get_plat_fwu_metadata_ops(void);
> +int fwu_get_active_index(u32 *active_idx);
> +int fwu_update_active_index(u32 active_idx);
> +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 *nparts);
> +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> +			  int *alt_num);
> +int fwu_metadata_check(void);
> +int fwu_revert_boot_index(u32 *active_idx);
> +int fwu_accept_image(efi_guid_t *img_type_id);
> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
> +int fwu_get_metadata(struct fwu_metadata **metadata);
> +
> +#endif /* _FWU_METADATA_H_ */
> diff --git a/lib/fwu_updates/fwu_metadata.c b/lib/fwu_updates/fwu_metadata.c
> new file mode 100644
> index 0000000000..ebc3eaa04a
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_metadata.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#include <fwu_metadata.h>
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +
> +static inline struct fwu_metadata_ops *get_fwu_metadata_ops(void)
> +{
> +	return get_plat_fwu_metadata_ops();
> +}
> +
> +/**
> + * fwu_get_active_index() - Get active_index from the metadata
> + * @active_idx: active_index value to be read
> + *
> + * Read the active_index field from the metadata and place it in
> + * the variable pointed to be the function argument.
> + *
> + * Return: 0 if OK, -ve on error

-ve ?

> + *
> + */
> +int fwu_get_active_index(u32 *active_idx)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();

On all callsites of get_fwu_metadata_ops() do we need to be that verbose on
the ops missing?  If not we can just squeeze in the if
(!ops->XXXXXX) check in get_fwu_metadata_ops() and simply return an error
there.

> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_update_active_index(u32 active_idx)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->update_active_index) {
> +		log_err("update_active_index() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->update_active_index(active_idx);
> +}
> +
> +/**
> + * fwu_fill_partition_guid_array() - Fill the part_guid_arr array with the guid's of
> + *                                   the partitions
> + * @part_guid_arr: array of partition guid's
> + * @nparts: Number of gpt partitions on the device
> + *
> + * Get the information on the partition guid's, filling the array with the guid
> + * values and also the number of partitions.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 *nparts)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->fill_partition_guid_array) {
> +		log_err("fill_partition_guid_array() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->fill_partition_guid_array(part_guid_arr, nparts);
> +}
> +
> +/**
> + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
> + * @image_type_id: image guid as passed in the capsule
> + * @update_bank: Bank to which the update is to be made
> + * @alt_num: The alt_num for the image
> + *
> + * Based on the guid value passed in the capsule, along with the bank to which the
> + * image needs to be updated, get the dfu alt number which will be used for the
> + * capsule update
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> +			  int *alt_num)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->get_image_alt_num) {
> +		log_err("get_image_alt_num() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_image_alt_num(image_type_id, update_bank, alt_num);
> +}
> +
> +/**
> + * fwu_metadata_check() - Check if the metadata is valid
> + *
> + * Validate both copies of metadata. If one of the copies
> + * has gone bad, restore it from the other bad copy.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_metadata_check(void)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->metadata_check) {
> +		log_err("metadata_check() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->metadata_check();
> +}
> +
> +/**
> + * fwu_revert_boot_index() - Revert the active index in the metadata
> + * @active_idx: Value of the updated active_index
> + *
> + * Revert the active_index value in the metadata, by swapping the values
> + * of active_index and previous_active_index in both copies of the
> + * metadata.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_revert_boot_index(u32 *active_idx)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->revert_boot_index) {
> +		log_err("revert_boot_index() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->revert_boot_index(active_idx);
> +}
> +
> +/**
> + * fwu_accept_image() - Set the Acceptance bit for the image
> + * @img_type_id: Guid of the image type for which the accepted bit is to be
> + *               cleared
> + *
> + * Set the accepted bit for the image specified by the img_guid parameter. This
> + * indicates acceptance of image for subsequent boots by some governing component
> + * like OS(or firmware).
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_accept_image(efi_guid_t *img_type_id)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->set_accept_image) {
> +		log_err("set_accept_image() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->set_accept_image(img_type_id);
> +}
> +
> +/**
> + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> + * @img_type_id: Guid of the image type for which the accepted bit is to be
> + *               cleared
> + *
> + * Clear the accepted bit for the image type specified by the img_type_id parameter.
> + * This function is called after the image has been updated. The accepted bit is
> + * cleared to be set subsequently after passing the image acceptance criteria, by
> + * either the OS(or firmware)
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->clear_accept_image) {
> +		log_err("clear_accept_image() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->clear_accept_image(img_type_id, bank);
> +}
> +
> +/**
> + * fwu_get_metadata() - Get a metadata copy
> + * @metadata: Copy of the metadata
> + *
> + * Get a valid copy of the metadata.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_metadata(struct fwu_metadata **metadata)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->get_metadata) {
> +		log_err("get_metadata() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_metadata(metadata);
> +}
> -- 
> 2.17.1
> 


Cheers
/Ilias

hi Ilias,
Thanks for the review.

On Fri, 26 Nov 2021 at 17:05, Ilias Apalodimas <ilias.apalodimas@linaro.org>
wrote:

> Hi Sughosh,
>
> On Thu, Nov 25, 2021 at 12:42:55PM +0530, Sughosh Ganu wrote:
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, which is stored on
> > a dedicated partition. Add the metadata structure, and functions to
> > access the metadata. These are generic API's, and implementations can
> > be added based on parameters like how the metadata partition is
> > accessed and what type of storage device houses the metadata.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  include/fwu_metadata.h         | 125 +++++++++++++++
> >  lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
> >  2 files changed, 400 insertions(+)
> >  create mode 100644 include/fwu_metadata.h
> >  create mode 100644 lib/fwu_updates/fwu_metadata.c
> >
> > diff --git a/include/fwu_metadata.h b/include/fwu_metadata.h
> > new file mode 100644
> > index 0000000000..e692ef7506
> > --- /dev/null
> > +++ b/include/fwu_metadata.h
> > @@ -0,0 +1,125 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
>
> Please add a link here to the arm spec that describes the metadata etc
>

Will add.


>
> > + */
> > +
> > +#if !defined _FWU_METADATA_H_
> > +#define _FWU_METADATA_H_
> > +
> > +#include <blk.h>
> > +#include <efi.h>
> > +#include <uuid.h>
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct fwu_image_bank_info - firmware image information
> > + * @image_uuid: Guid value of the image in this bank
> > + * @accepted: Acceptance status of the image
> > + * @reserved: Reserved
> > + *
> > + * The structure contains image specific fields which are
> > + * used to identify the image and to specify the image's
> > + * acceptance status
> > + */
> > +struct fwu_image_bank_info {
> > +     efi_guid_t  image_uuid;
> > +     u32 accepted;
> > +     u32 reserved;
> > +};
>
> fwu_image_bank_info -> fwu_img_bank_info
>

Okay.


>
> > +
> > +/**
> > + * struct fwu_image_entry - information for a particular type of image
> > + * @image_type_uuid: Guid value for identifying the image type
> > + * @location_uuid: Guid of the storage volume where the image is located
>
> /s/Guid/GUID
>

Will change.


>
> > + * @img_bank_info: Array containing properties of images
> > + *
> > + * This structure contains information on various types of updatable
> > + * firmware images. Each image type then contains an array of image
> > + * information per bank.
> > + */
> > +struct fwu_image_entry {
> > +     efi_guid_t image_type_uuid;
> > +     efi_guid_t location_uuid;
> > +     struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> > +};
> > +
>
> It seems like you've followed the naming proposed in the spec,  which makes
> reading spec -- code easier.  However I feel we should add a few more
> comments on the naming to make reading easier or change the naming and
> mention the original name in comments.
>
> A 'bank' is supposed to contain:
> bank[0]: Uboot(0), TF-A(0) etc
> bank[1]: Uboot(1), TF-A(1) etc
> However there's no structure that defines an entire bank.  Instead the bank
> information is constructed by reading the metadata and fixing it up on
> the fly.
>
> fwu_image_bank_info -- Information for a specific image (e.g OP-TEE,
> U-Boot, TF-A, whatever) but not within a *bank*.  That's amongst a
> collection of images of the same type.
>
> IOW img_bank_info looks like:
> img_bank_info[0] -> U-Boot(0), U-Boot(1) etc
> img_bank_info[1] -> TF-A(0), TF-A(1) etc
>
> @Jose can we tweak the spec naming a bit to be more intuitive?
> I am terrible at naming stuff but what about:
> fwu_image_bank_info -> fwu_img_repo_info, fwu_img_vault_info,
>                                            fwu_img_storage_info,
> fwu_img_array_info,


>
>
> > +/**
> > + * struct fwu_metadata - Metadata structure for multi-bank updates
> > + * @crc32: crc32 value for the metadata
> > + * @version: Metadata version
> > + * @active_index: Index of the bank currently used for booting images
> > + * @previous_active_inde: Index of the bank used before the current bank
> > + *                        being used for booting
> > + * @img_entry: Array of information on various firmware images that can
> > + *             be updated
> > + *
> > + * This structure is used to store all the needed information for
> performing
> > + * multi bank updates on the platform. This contains info on the bank
> being
> > + * used to boot along with the information needed for identification of
> > + * individual images
> > + */
> > +struct fwu_metadata {
> > +     u32 crc32;
> > +     u32 version;
> > +     u32 active_index;
> > +     u32 previous_active_index;
> > +
> > +     struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> > +};
> > +
> > +/**
> > + * @get_active_index: get the current active_index value
> > + * @update_active_index: update the active_index value
> > + * @fill_partition_guid_array: fill the array with guid values of the
> > + *                             partitions found on the storage media
> > + * @get_image_alt_num: get the alt number to be used for the image
> > + * @metadata_check: check the validity of the metadata partitions
> > + * @revert_boot_index: set the active_index to previous_active_index
> > + * @set_accept_image: set the accepted bit for the image
> > + * @clear_accept_image: clear the accepted bit for the image
> > + * @get_metadata() - Get a metadata copy
> > + */
> > +struct fwu_metadata_ops {
> > +     int (*get_active_index)(u32 *active_idx);
> > +
> > +     int (*update_active_index)(u32 active_idx);
> > +
> > +     int (*fill_partition_guid_array)(efi_guid_t **part_guid_arr,
> > +                                      u32 *nparts);
> > +
> > +     int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> > +                              int *alt_num);
> > +
> > +     int (*metadata_check)(void);
> > +
> > +     int (*revert_boot_index)(u32 *active_idx);
> > +
> > +     int (*set_accept_image)(efi_guid_t *img_type_id);
> > +
> > +     int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> > +
> > +     int (*get_metadata)(struct fwu_metadata **metadata);
> > +};
> > +
> > +#define FWU_METADATA_GUID \
> > +     EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> > +              0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> > +
> > +#define FWU_METADATA_VERSION 0x1
> > +
> > +extern struct fwu_metadata_ops fwu_gpt_blk_ops;
> > +
> > +struct fwu_metadata_ops *get_plat_fwu_metadata_ops(void);
> > +int fwu_get_active_index(u32 *active_idx);
> > +int fwu_update_active_index(u32 active_idx);
> > +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32
> *nparts);
> > +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> > +                       int *alt_num);
> > +int fwu_metadata_check(void);
> > +int fwu_revert_boot_index(u32 *active_idx);
> > +int fwu_accept_image(efi_guid_t *img_type_id);
> > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
> > +int fwu_get_metadata(struct fwu_metadata **metadata);
> > +
> > +#endif /* _FWU_METADATA_H_ */
> > diff --git a/lib/fwu_updates/fwu_metadata.c
> b/lib/fwu_updates/fwu_metadata.c
> > new file mode 100644
> > index 0000000000..ebc3eaa04a
> > --- /dev/null
> > +++ b/lib/fwu_updates/fwu_metadata.c
> > @@ -0,0 +1,275 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#include <fwu_metadata.h>
> > +
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +
> > +static inline struct fwu_metadata_ops *get_fwu_metadata_ops(void)
> > +{
> > +     return get_plat_fwu_metadata_ops();
> > +}
> > +
> > +/**
> > + * fwu_get_active_index() - Get active_index from the metadata
> > + * @active_idx: active_index value to be read
> > + *
> > + * Read the active_index field from the metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
>
> -ve ?
>

Sorry, I did not get this review comment. The active index is returned back
through the function parameter. The return value indicates whether the
function is returning the active_index value successfully(0), or if there
was an error(-ve) in getting the active_index.


> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idx)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
>
> On all callsites of get_fwu_metadata_ops() do we need to be that verbose on
> the ops missing?  If not we can just squeeze in the if
> (!ops->XXXXXX) check in get_fwu_metadata_ops() and simply return an error
> there.
>

Okay. Will change.


>
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_update_active_index(u32 active_idx)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->update_active_index) {
> > +             log_err("update_active_index() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->update_active_index(active_idx);
> > +}
> > +
> > +/**
> > + * fwu_fill_partition_guid_array() - Fill the part_guid_arr array with
> the guid's of
> > + *                                   the partitions
> > + * @part_guid_arr: array of partition guid's
> > + * @nparts: Number of gpt partitions on the device
> > + *
> > + * Get the information on the partition guid's, filling the array with
> the guid
> > + * values and also the number of partitions.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32
> *nparts)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->fill_partition_guid_array) {
> > +             log_err("fill_partition_guid_array() method not defined
> for the platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->fill_partition_guid_array(part_guid_arr, nparts);
> > +}
> > +
> > +/**
> > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for
> capsule update
> > + * @image_type_id: image guid as passed in the capsule
> > + * @update_bank: Bank to which the update is to be made
> > + * @alt_num: The alt_num for the image
> > + *
> > + * Based on the guid value passed in the capsule, along with the bank
> to which the
> > + * image needs to be updated, get the dfu alt number which will be used
> for the
> > + * capsule update
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> > +                       int *alt_num)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->get_image_alt_num) {
> > +             log_err("get_image_alt_num() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->get_image_alt_num(image_type_id, update_bank, alt_num);
> > +}
> > +
> > +/**
> > + * fwu_metadata_check() - Check if the metadata is valid
> > + *
> > + * Validate both copies of metadata. If one of the copies
> > + * has gone bad, restore it from the other bad copy.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_metadata_check(void)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->metadata_check) {
> > +             log_err("metadata_check() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->metadata_check();
> > +}
> > +
> > +/**
> > + * fwu_revert_boot_index() - Revert the active index in the metadata
> > + * @active_idx: Value of the updated active_index
> > + *
> > + * Revert the active_index value in the metadata, by swapping the values
> > + * of active_index and previous_active_index in both copies of the
> > + * metadata.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_revert_boot_index(u32 *active_idx)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->revert_boot_index) {
> > +             log_err("revert_boot_index() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->revert_boot_index(active_idx);
> > +}
> > +
> > +/**
> > + * fwu_accept_image() - Set the Acceptance bit for the image
> > + * @img_type_id: Guid of the image type for which the accepted bit is
> to be
> > + *               cleared
> > + *
> > + * Set the accepted bit for the image specified by the img_guid
> parameter. This
> > + * indicates acceptance of image for subsequent boots by some governing
> component
> > + * like OS(or firmware).
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_accept_image(efi_guid_t *img_type_id)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->set_accept_image) {
> > +             log_err("set_accept_image() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->set_accept_image(img_type_id);
> > +}
> > +
> > +/**
> > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> > + * @img_type_id: Guid of the image type for which the accepted bit is
> to be
> > + *               cleared
> > + *
> > + * Clear the accepted bit for the image type specified by the
> img_type_id parameter.
> > + * This function is called after the image has been updated. The
> accepted bit is
> > + * cleared to be set subsequently after passing the image acceptance
> criteria, by
> > + * either the OS(or firmware)
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->clear_accept_image) {
> > +             log_err("clear_accept_image() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->clear_accept_image(img_type_id, bank);
> > +}
> > +
> > +/**
> > + * fwu_get_metadata() - Get a metadata copy
> > + * @metadata: Copy of the metadata
> > + *
> > + * Get a valid copy of the metadata.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_metadata(struct fwu_metadata **metadata)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->get_metadata) {
> > +             log_err("get_metadata() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->get_metadata(metadata);
> > +}
> > --
> > 2.17.1
> >
>
>
> Cheers
> /Ilias
>

On 11/25/21 08:12, Sughosh Ganu wrote:
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, which is stored on
> a dedicated partition. Add the metadata structure, and functions to
> access the metadata. These are generic API's, and implementations can
> be added based on parameters like how the metadata partition is
> accessed and what type of storage device houses the metadata.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   include/fwu_metadata.h         | 125 +++++++++++++++
>   lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
>   2 files changed, 400 insertions(+)
>   create mode 100644 include/fwu_metadata.h
>   create mode 100644 lib/fwu_updates/fwu_metadata.c
>
> diff --git a/include/fwu_metadata.h b/include/fwu_metadata.h
> new file mode 100644
> index 0000000000..e692ef7506
> --- /dev/null
> +++ b/include/fwu_metadata.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#if !defined _FWU_METADATA_H_
> +#define _FWU_METADATA_H_
> +
> +#include <blk.h>
> +#include <efi.h>
> +#include <uuid.h>
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct fwu_image_bank_info - firmware image information
> + * @image_uuid: Guid value of the image in this bank
> + * @accepted: Acceptance status of the image
> + * @reserved: Reserved
> + *
> + * The structure contains image specific fields which are
> + * used to identify the image and to specify the image's
> + * acceptance status
> + */
> +struct fwu_image_bank_info {
> +	efi_guid_t  image_uuid;
> +	u32 accepted;
> +	u32 reserved;
> +};
> +
> +/**
> + * struct fwu_image_entry - information for a particular type of image
> + * @image_type_uuid: Guid value for identifying the image type
> + * @location_uuid: Guid of the storage volume where the image is located
> + * @img_bank_info: Array containing properties of images
> + *
> + * This structure contains information on various types of updatable
> + * firmware images. Each image type then contains an array of image
> + * information per bank.
> + */
> +struct fwu_image_entry {
> +	efi_guid_t image_type_uuid;
> +	efi_guid_t location_uuid;
> +	struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> +};
> +
> +/**
> + * struct fwu_metadata - Metadata structure for multi-bank updates
> + * @crc32: crc32 value for the metadata
> + * @version: Metadata version
> + * @active_index: Index of the bank currently used for booting images
> + * @previous_active_inde: Index of the bank used before the current bank
> + *                        being used for booting
> + * @img_entry: Array of information on various firmware images that can
> + *             be updated
> + *
> + * This structure is used to store all the needed information for performing
> + * multi bank updates on the platform. This contains info on the bank being
> + * used to boot along with the information needed for identification of
> + * individual images
> + */
> +struct fwu_metadata {
> +	u32 crc32;
> +	u32 version;
> +	u32 active_index;
> +	u32 previous_active_index;
> +
> +	struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> +};
> +
> +/**
> + * @get_active_index: get the current active_index value
> + * @update_active_index: update the active_index value
> + * @fill_partition_guid_array: fill the array with guid values of the
> + *                             partitions found on the storage media
> + * @get_image_alt_num: get the alt number to be used for the image
> + * @metadata_check: check the validity of the metadata partitions
> + * @revert_boot_index: set the active_index to previous_active_index
> + * @set_accept_image: set the accepted bit for the image
> + * @clear_accept_image: clear the accepted bit for the image
> + * @get_metadata() - Get a metadata copy
> + */
> +struct fwu_metadata_ops {
> +	int (*get_active_index)(u32 *active_idx);
> +
> +	int (*update_active_index)(u32 active_idx);
> +
> +	int (*fill_partition_guid_array)(efi_guid_t **part_guid_arr,
> +					 u32 *nparts);
> +
> +	int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> +				 int *alt_num);
> +
> +	int (*metadata_check)(void);
> +
> +	int (*revert_boot_index)(u32 *active_idx);
> +
> +	int (*set_accept_image)(efi_guid_t *img_type_id);
> +
> +	int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> +
> +	int (*get_metadata)(struct fwu_metadata **metadata);
> +};
> +
> +#define FWU_METADATA_GUID \
> +	EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> +		 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> +
> +#define FWU_METADATA_VERSION	0x1
> +
> +extern struct fwu_metadata_ops fwu_gpt_blk_ops;
> +
> +struct fwu_metadata_ops *get_plat_fwu_metadata_ops(void);
> +int fwu_get_active_index(u32 *active_idx);
> +int fwu_update_active_index(u32 active_idx);
> +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 *nparts);
> +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> +			  int *alt_num);
> +int fwu_metadata_check(void);
> +int fwu_revert_boot_index(u32 *active_idx);
> +int fwu_accept_image(efi_guid_t *img_type_id);
> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
> +int fwu_get_metadata(struct fwu_metadata **metadata);
> +
> +#endif /* _FWU_METADATA_H_ */
> diff --git a/lib/fwu_updates/fwu_metadata.c b/lib/fwu_updates/fwu_metadata.c
> new file mode 100644
> index 0000000000..ebc3eaa04a
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_metadata.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#include <fwu_metadata.h>
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +
> +static inline struct fwu_metadata_ops *get_fwu_metadata_ops(void)
> +{
> +	return get_plat_fwu_metadata_ops();
> +}
> +
> +/**
> + * fwu_get_active_index() - Get active_index from the metadata
> + * @active_idx: active_index value to be read
> + *
> + * Read the active_index field from the metadata and place it in
> + * the variable pointed to be the function argument.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_active_index(u32 *active_idx)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->get_active_index) {
> +		log_err("get_active_index() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_active_index(active_idx);
> +}
> +
> +/**
> + * fwu_update_active_index() - Update active_index from the metadata
> + * @active_idx: active_index value to be updated
> + *
> + * Update the active_index field in the metadata
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_update_active_index(u32 active_idx)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->update_active_index) {
> +		log_err("update_active_index() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->update_active_index(active_idx);
> +}
> +
> +/**
> + * fwu_fill_partition_guid_array() - Fill the part_guid_arr array with the guid's of
> + *                                   the partitions
> + * @part_guid_arr: array of partition guid's
> + * @nparts: Number of gpt partitions on the device
> + *
> + * Get the information on the partition guid's, filling the array with the guid
> + * values and also the number of partitions.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 *nparts)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->fill_partition_guid_array) {
> +		log_err("fill_partition_guid_array() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->fill_partition_guid_array(part_guid_arr, nparts);
> +}
> +
> +/**
> + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
> + * @image_type_id: image guid as passed in the capsule
> + * @update_bank: Bank to which the update is to be made
> + * @alt_num: The alt_num for the image
> + *
> + * Based on the guid value passed in the capsule, along with the bank to which the
> + * image needs to be updated, get the dfu alt number which will be used for the
> + * capsule update
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> +			  int *alt_num)
> +{
> +	struct fwu_metadata_ops *ops;

The metadata is an untrusted information source and hence MUST NOT be
used to map the image_type_id to the DFU alt_number. Don't invite for an
denial of service attack.

The signed capsule would be a good place for storing the DFU mapping.

Best regards

Heinrich


> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->get_image_alt_num) {
> +		log_err("get_image_alt_num() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_image_alt_num(image_type_id, update_bank, alt_num);
> +}
> +
> +/**
> + * fwu_metadata_check() - Check if the metadata is valid
> + *
> + * Validate both copies of metadata. If one of the copies
> + * has gone bad, restore it from the other bad copy.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_metadata_check(void)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->metadata_check) {
> +		log_err("metadata_check() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->metadata_check();
> +}
> +
> +/**
> + * fwu_revert_boot_index() - Revert the active index in the metadata
> + * @active_idx: Value of the updated active_index
> + *
> + * Revert the active_index value in the metadata, by swapping the values
> + * of active_index and previous_active_index in both copies of the
> + * metadata.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_revert_boot_index(u32 *active_idx)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->revert_boot_index) {
> +		log_err("revert_boot_index() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->revert_boot_index(active_idx);
> +}
> +
> +/**
> + * fwu_accept_image() - Set the Acceptance bit for the image
> + * @img_type_id: Guid of the image type for which the accepted bit is to be
> + *               cleared
> + *
> + * Set the accepted bit for the image specified by the img_guid parameter. This
> + * indicates acceptance of image for subsequent boots by some governing component
> + * like OS(or firmware).
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_accept_image(efi_guid_t *img_type_id)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->set_accept_image) {
> +		log_err("set_accept_image() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->set_accept_image(img_type_id);
> +}
> +
> +/**
> + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> + * @img_type_id: Guid of the image type for which the accepted bit is to be
> + *               cleared
> + *
> + * Clear the accepted bit for the image type specified by the img_type_id parameter.
> + * This function is called after the image has been updated. The accepted bit is
> + * cleared to be set subsequently after passing the image acceptance criteria, by
> + * either the OS(or firmware)
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->clear_accept_image) {
> +		log_err("clear_accept_image() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->clear_accept_image(img_type_id, bank);
> +}
> +
> +/**
> + * fwu_get_metadata() - Get a metadata copy
> + * @metadata: Copy of the metadata
> + *
> + * Get a valid copy of the metadata.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_metadata(struct fwu_metadata **metadata)
> +{
> +	struct fwu_metadata_ops *ops;
> +
> +	ops = get_fwu_metadata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (!ops->get_metadata) {
> +		log_err("get_metadata() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_metadata(metadata);
> +}
>

On Tue, 30 Nov 2021 at 18:33, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 11/25/21 08:12, Sughosh Ganu wrote:
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, which is stored on
> > a dedicated partition. Add the metadata structure, and functions to
> > access the metadata. These are generic API's, and implementations can
> > be added based on parameters like how the metadata partition is
> > accessed and what type of storage device houses the metadata.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   include/fwu_metadata.h         | 125 +++++++++++++++
> >   lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
> >   2 files changed, 400 insertions(+)
> >   create mode 100644 include/fwu_metadata.h
> >   create mode 100644 lib/fwu_updates/fwu_metadata.c
> >
> > diff --git a/include/fwu_metadata.h b/include/fwu_metadata.h
> > new file mode 100644
> > index 0000000000..e692ef7506
> > --- /dev/null
> > +++ b/include/fwu_metadata.h
> > @@ -0,0 +1,125 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#if !defined _FWU_METADATA_H_
> > +#define _FWU_METADATA_H_
> > +
> > +#include <blk.h>
> > +#include <efi.h>
> > +#include <uuid.h>
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct fwu_image_bank_info - firmware image information
> > + * @image_uuid: Guid value of the image in this bank
> > + * @accepted: Acceptance status of the image
> > + * @reserved: Reserved
> > + *
> > + * The structure contains image specific fields which are
> > + * used to identify the image and to specify the image's
> > + * acceptance status
> > + */
> > +struct fwu_image_bank_info {
> > +     efi_guid_t  image_uuid;
> > +     u32 accepted;
> > +     u32 reserved;
> > +};
> > +
> > +/**
> > + * struct fwu_image_entry - information for a particular type of image
> > + * @image_type_uuid: Guid value for identifying the image type
> > + * @location_uuid: Guid of the storage volume where the image is located
> > + * @img_bank_info: Array containing properties of images
> > + *
> > + * This structure contains information on various types of updatable
> > + * firmware images. Each image type then contains an array of image
> > + * information per bank.
> > + */
> > +struct fwu_image_entry {
> > +     efi_guid_t image_type_uuid;
> > +     efi_guid_t location_uuid;
> > +     struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> > +};
> > +
> > +/**
> > + * struct fwu_metadata - Metadata structure for multi-bank updates
> > + * @crc32: crc32 value for the metadata
> > + * @version: Metadata version
> > + * @active_index: Index of the bank currently used for booting images
> > + * @previous_active_inde: Index of the bank used before the current bank
> > + *                        being used for booting
> > + * @img_entry: Array of information on various firmware images that can
> > + *             be updated
> > + *
> > + * This structure is used to store all the needed information for
> performing
> > + * multi bank updates on the platform. This contains info on the bank
> being
> > + * used to boot along with the information needed for identification of
> > + * individual images
> > + */
> > +struct fwu_metadata {
> > +     u32 crc32;
> > +     u32 version;
> > +     u32 active_index;
> > +     u32 previous_active_index;
> > +
> > +     struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> > +};
> > +
> > +/**
> > + * @get_active_index: get the current active_index value
> > + * @update_active_index: update the active_index value
> > + * @fill_partition_guid_array: fill the array with guid values of the
> > + *                             partitions found on the storage media
> > + * @get_image_alt_num: get the alt number to be used for the image
> > + * @metadata_check: check the validity of the metadata partitions
> > + * @revert_boot_index: set the active_index to previous_active_index
> > + * @set_accept_image: set the accepted bit for the image
> > + * @clear_accept_image: clear the accepted bit for the image
> > + * @get_metadata() - Get a metadata copy
> > + */
> > +struct fwu_metadata_ops {
> > +     int (*get_active_index)(u32 *active_idx);
> > +
> > +     int (*update_active_index)(u32 active_idx);
> > +
> > +     int (*fill_partition_guid_array)(efi_guid_t **part_guid_arr,
> > +                                      u32 *nparts);
> > +
> > +     int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> > +                              int *alt_num);
> > +
> > +     int (*metadata_check)(void);
> > +
> > +     int (*revert_boot_index)(u32 *active_idx);
> > +
> > +     int (*set_accept_image)(efi_guid_t *img_type_id);
> > +
> > +     int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> > +
> > +     int (*get_metadata)(struct fwu_metadata **metadata);
> > +};
> > +
> > +#define FWU_METADATA_GUID \
> > +     EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> > +              0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> > +
> > +#define FWU_METADATA_VERSION 0x1
> > +
> > +extern struct fwu_metadata_ops fwu_gpt_blk_ops;
> > +
> > +struct fwu_metadata_ops *get_plat_fwu_metadata_ops(void);
> > +int fwu_get_active_index(u32 *active_idx);
> > +int fwu_update_active_index(u32 active_idx);
> > +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32
> *nparts);
> > +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> > +                       int *alt_num);
> > +int fwu_metadata_check(void);
> > +int fwu_revert_boot_index(u32 *active_idx);
> > +int fwu_accept_image(efi_guid_t *img_type_id);
> > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
> > +int fwu_get_metadata(struct fwu_metadata **metadata);
> > +
> > +#endif /* _FWU_METADATA_H_ */
> > diff --git a/lib/fwu_updates/fwu_metadata.c
> b/lib/fwu_updates/fwu_metadata.c
> > new file mode 100644
> > index 0000000000..ebc3eaa04a
> > --- /dev/null
> > +++ b/lib/fwu_updates/fwu_metadata.c
> > @@ -0,0 +1,275 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#include <fwu_metadata.h>
> > +
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +
> > +static inline struct fwu_metadata_ops *get_fwu_metadata_ops(void)
> > +{
> > +     return get_plat_fwu_metadata_ops();
> > +}
> > +
> > +/**
> > + * fwu_get_active_index() - Get active_index from the metadata
> > + * @active_idx: active_index value to be read
> > + *
> > + * Read the active_index field from the metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idx)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->get_active_index) {
> > +             log_err("get_active_index() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->get_active_index(active_idx);
> > +}
> > +
> > +/**
> > + * fwu_update_active_index() - Update active_index from the metadata
> > + * @active_idx: active_index value to be updated
> > + *
> > + * Update the active_index field in the metadata
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_update_active_index(u32 active_idx)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->update_active_index) {
> > +             log_err("update_active_index() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->update_active_index(active_idx);
> > +}
> > +
> > +/**
> > + * fwu_fill_partition_guid_array() - Fill the part_guid_arr array with
> the guid's of
> > + *                                   the partitions
> > + * @part_guid_arr: array of partition guid's
> > + * @nparts: Number of gpt partitions on the device
> > + *
> > + * Get the information on the partition guid's, filling the array with
> the guid
> > + * values and also the number of partitions.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32
> *nparts)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->fill_partition_guid_array) {
> > +             log_err("fill_partition_guid_array() method not defined
> for the platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->fill_partition_guid_array(part_guid_arr, nparts);
> > +}
> > +
> > +/**
> > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for
> capsule update
> > + * @image_type_id: image guid as passed in the capsule
> > + * @update_bank: Bank to which the update is to be made
> > + * @alt_num: The alt_num for the image
> > + *
> > + * Based on the guid value passed in the capsule, along with the bank
> to which the
> > + * image needs to be updated, get the dfu alt number which will be used
> for the
> > + * capsule update
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> > +                       int *alt_num)
> > +{
> > +     struct fwu_metadata_ops *ops;
>
> The metadata is an untrusted information source and hence MUST NOT be
> used to map the image_type_id to the DFU alt_number. Don't invite for an
> denial of service attack.
>
> The signed capsule would be a good place for storing the DFU mapping.
>

I understand your concern with using dfu_alt_info for storing the
information needed for writing the capsule payload. However, putting the
information currently stored on the dfu_alt_info on a capsule should
require a spec change IMO. This should first be discussed and brought in as
part of the UEFI spec. Also, when you say signed capsule, please note not
the entire capsule gets signed -- it is only the capsule payloads that are
signed, not the headers. So putting the information currently stored in dfu
env var to the capsule would mean adding a header to the payload, which
would contain this information, and then the header plus payload would be
signed. However this is implemented, this would mean changes to the current
capsule format, and making this change without changing the spec would also
mean that we will also not be able to use the GenerateCapsule tool for
capsule generation. This is not a small change which can be included as a
patch in the FWU A/B update series, but should be taken up as a separate
exercise.

-sughosh


> Best regards
>
> Heinrich
>
>
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->get_image_alt_num) {
> > +             log_err("get_image_alt_num() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->get_image_alt_num(image_type_id, update_bank, alt_num);
> > +}
> > +
> > +/**
> > + * fwu_metadata_check() - Check if the metadata is valid
> > + *
> > + * Validate both copies of metadata. If one of the copies
> > + * has gone bad, restore it from the other bad copy.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_metadata_check(void)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->metadata_check) {
> > +             log_err("metadata_check() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->metadata_check();
> > +}
> > +
> > +/**
> > + * fwu_revert_boot_index() - Revert the active index in the metadata
> > + * @active_idx: Value of the updated active_index
> > + *
> > + * Revert the active_index value in the metadata, by swapping the values
> > + * of active_index and previous_active_index in both copies of the
> > + * metadata.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_revert_boot_index(u32 *active_idx)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->revert_boot_index) {
> > +             log_err("revert_boot_index() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->revert_boot_index(active_idx);
> > +}
> > +
> > +/**
> > + * fwu_accept_image() - Set the Acceptance bit for the image
> > + * @img_type_id: Guid of the image type for which the accepted bit is
> to be
> > + *               cleared
> > + *
> > + * Set the accepted bit for the image specified by the img_guid
> parameter. This
> > + * indicates acceptance of image for subsequent boots by some governing
> component
> > + * like OS(or firmware).
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_accept_image(efi_guid_t *img_type_id)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->set_accept_image) {
> > +             log_err("set_accept_image() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->set_accept_image(img_type_id);
> > +}
> > +
> > +/**
> > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> > + * @img_type_id: Guid of the image type for which the accepted bit is
> to be
> > + *               cleared
> > + *
> > + * Clear the accepted bit for the image type specified by the
> img_type_id parameter.
> > + * This function is called after the image has been updated. The
> accepted bit is
> > + * cleared to be set subsequently after passing the image acceptance
> criteria, by
> > + * either the OS(or firmware)
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->clear_accept_image) {
> > +             log_err("clear_accept_image() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->clear_accept_image(img_type_id, bank);
> > +}
> > +
> > +/**
> > + * fwu_get_metadata() - Get a metadata copy
> > + * @metadata: Copy of the metadata
> > + *
> > + * Get a valid copy of the metadata.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_metadata(struct fwu_metadata **metadata)
> > +{
> > +     struct fwu_metadata_ops *ops;
> > +
> > +     ops = get_fwu_metadata_ops();
> > +     if (!ops) {
> > +             log_err("Unable to get fwu ops\n");
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (!ops->get_metadata) {
> > +             log_err("get_metadata() method not defined for the
> platform\n");
> > +             return -ENOSYS;
> > +     }
> > +
> > +     return ops->get_metadata(metadata);
> > +}
> >
>
>

Hi,

On Thu, 25 Nov 2021 at 00:13, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, which is stored on
> a dedicated partition. Add the metadata structure, and functions to
> access the metadata. These are generic API's, and implementations can
> be added based on parameters like how the metadata partition is
> accessed and what type of storage device houses the metadata.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/fwu_metadata.h         | 125 +++++++++++++++
>  lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
>  2 files changed, 400 insertions(+)
>  create mode 100644 include/fwu_metadata.h
>  create mode 100644 lib/fwu_updates/fwu_metadata.c

What is this, please? Could you add a link to the docs in the commit
message and something in doc/ as well?

Regards,
Simon

hi Simon,

On Wed, 1 Dec 2021 at 11:56, Simon Glass <sjg@chromium.org> wrote:

> Hi,
>
> On Thu, 25 Nov 2021 at 00:13, Sughosh Ganu <sughosh.ganu@linaro.org>
> wrote:
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, which is stored on
> > a dedicated partition. Add the metadata structure, and functions to
> > access the metadata. These are generic API's, and implementations can
> > be added based on parameters like how the metadata partition is
> > accessed and what type of storage device houses the metadata.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  include/fwu_metadata.h         | 125 +++++++++++++++
> >  lib/fwu_updates/fwu_metadata.c | 275 +++++++++++++++++++++++++++++++++
> >  2 files changed, 400 insertions(+)
> >  create mode 100644 include/fwu_metadata.h
> >  create mode 100644 lib/fwu_updates/fwu_metadata.c
>
> What is this, please? Could you add a link to the docs in the commit
> message and something in doc/ as well?
>

This is implementation of the FWU[1] specification from Arm. Some aspects
of the DependableBoot[2] specification are also implemented. I had put
pointers to the document in the cover letter[3]. I have not added any
documentation yet, since this is a rfc. Will do so once the initial reviews
are completed. Hope that is fine.

-sughosh

[1] - https://developer.arm.com/documentation/den0118/a
[2] -
https://staging-git.codelinaro.org/linaro/firmware-dual-banked-updates/test
[3] - https://lists.denx.de/pipermail/u-boot/2021-November/468232.html

>
> Regards,
> Simon
>

Hi Heinrich,

[...]

> > +/**
> > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
> > + * @image_type_id: image guid as passed in the capsule
> > + * @update_bank: Bank to which the update is to be made
> > + * @alt_num: The alt_num for the image
> > + *
> > + * Based on the guid value passed in the capsule, along with the bank to which the
> > + * image needs to be updated, get the dfu alt number which will be used for the
> > + * capsule update
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> > +                       int *alt_num)
> > +{
> > +     struct fwu_metadata_ops *ops;
>
> The metadata is an untrusted information source and hence MUST NOT be
> used to map the image_type_id to the DFU alt_number. Don't invite for an
> denial of service attack.

You are assuming here an attacker can manipulate the metada to trigger
a DoS by overwriting wrong parts of the flash.  However there's an
easier way to trigger that.  If he already has access,  he can
completely erase the metadata and their backup GPT.  You then get the
same result a device that cant boot.  Is there any scenario you have
in mind that storing those as part fo the capsule would help?  The way
I see it unless we have the metadata and the firmware stored in a
flash in the secure world, I don't see a sensible way to protect
against those kind of attacks.

>
> The signed capsule would be a good place for storing the DFU mapping.
>


[...]

Thanks for taking the time with this!
Regards
/Ilias

Hi Sughosh,

[...]
>> > +{
>> > +     struct fwu_metadata_ops *ops;
>>
>> The metadata is an untrusted information source and hence MUST NOT be
>> used to map the image_type_id to the DFU alt_number. Don't invite for an
>> denial of service attack.
>>
>> The signed capsule would be a good place for storing the DFU mapping.
>
>
> I understand your concern with using dfu_alt_info for storing the information needed for writing the capsule payload. However, putting the information currently stored on the dfu_alt_info on a capsule should require a spec change IMO. This should first be discussed and brought in as part of the UEFI spec.

Well not the UEFI spec.  You got the FMP driver which is abstract
enough to handle that.  However as I already replied to Heinrich and
attacker can just erase the entire GPT,  instead of bothering altering
it.  So what I've been trying to think based on Heinrich's suggestion
is if an attacker can manipulate the metadata in such a way to force
the device boot something it shouldn't.  But since BL1 will go ahead
and verify signatures before booting them anyway,  I can't think of
something valid.

> Also, when you say signed capsule, please note not the entire capsule gets signed -- it is only the capsule payloads that are signed, not the headers. So putting the information currently stored in dfu env var to the capsule would mean adding a header to the payload, which would contain this information, and then the header plus payload would be signed. However this is > implemented, this would mean changes to the current capsule format, and making this change without changing the spec would also mean that we will also not be able to use the GenerateCapsule tool for capsule generation. This is not a small change which can be included as a patch in the FWU A/B update series, but should be taken up as a separate exercise.
>

[...]


Cheers
/Ilias

hi Ilias,

On Wed, 1 Dec 2021 at 13:20, Ilias Apalodimas <ilias.apalodimas@linaro.org>
wrote:

> Hi Sughosh,
>
> [...]
> >> > +{
> >> > +     struct fwu_metadata_ops *ops;
> >>
> >> The metadata is an untrusted information source and hence MUST NOT be
> >> used to map the image_type_id to the DFU alt_number. Don't invite for an
> >> denial of service attack.
> >>
> >> The signed capsule would be a good place for storing the DFU mapping.
> >
> >
> > I understand your concern with using dfu_alt_info for storing the
> information needed for writing the capsule payload. However, putting the
> information currently stored on the dfu_alt_info on a capsule should
> require a spec change IMO. This should first be discussed and brought in as
> part of the UEFI spec.
>
> Well not the UEFI spec.  You got the FMP driver which is abstract
> enough to handle that.  However as I already replied to Heinrich and
> attacker can just erase the entire GPT,  instead of bothering altering
> it.  So what I've been trying to think based on Heinrich's suggestion
> is if an attacker can manipulate the metadata in such a way to force
> the device boot something it shouldn't.  But since BL1 will go ahead
> and verify signatures before booting them anyway,  I can't think of
> something valid.
>

Sorry, I misinterpreted the comment from Heinrich. I was replying to the
comment from Heinrich about not using the dfu_alt_info env variable for the
updates. I think Heinrich is also suggesting putting the metadata
equivalent information on the capsule. This would also mean adding a header
to each payload where the header stores the metadata information. But as
you say, having the firmware and the metadata on a device that can be
accessed from the non-secure world, we cannot avoid DoS attacks even with
the metadata on the capsule. Also, this would mean having multiple copies
of the metadata, since the earlier stage bootloader(BL2/spl) shall still
need the metadata on a storage device partition to identify which bank to
boot from.

-sughosh


>
> > Also, when you say signed capsule, please note not the entire capsule
> gets signed -- it is only the capsule payloads that are signed, not the
> headers. So putting the information currently stored in dfu env var to the
> capsule would mean adding a header to the payload, which would contain this
> information, and then the header plus payload would be signed. However this
> is > implemented, this would mean changes to the current capsule format,
> and making this change without changing the spec would also mean that we
> will also not be able to use the GenerateCapsule tool for capsule
> generation. This is not a small change which can be included as a patch in
> the FWU A/B update series, but should be taken up as a separate exercise.
> >
>
> [...]
>
>
> Cheers
> /Ilias
>