[Skiboot] [PATCH V6 17/21] core/pldm: Update "bmc-firmware-version" device-tree field

[Frederic Barrat]

On 13/09/2022 12:27, Christophe Lombard wrote:
> Use the GetFruRecordByOptionReq command to retrieve the bmc information
> with: "FRU Field Type": Version
>        "FRU Record Set Identifier": 1,
>        "FRU Record Type": "General(1)"
> and update the "bmc-firmware-version" device-tree field.
> 
> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
> ---
>   core/pldm/pldm-fru-requests.c | 60 +++++++++++++++++++++++++++++++++++
>   core/pldm/pldm.h              |  1 +
>   include/pldm.h                |  5 +++
>   3 files changed, 66 insertions(+)
> 
> diff --git a/core/pldm/pldm-fru-requests.c b/core/pldm/pldm-fru-requests.c
> index 02c9b968..b07a7b1a 100644
> --- a/core/pldm/pldm-fru-requests.c
> +++ b/core/pldm/pldm-fru-requests.c
> @@ -15,6 +15,7 @@ static void *fru_record_table;
>   static size_t fru_record_length;
>   
>   static bool fru_ready;
> +static char *bmc_version = NULL;
>   
>   static void fru_init_complete(bool success)
>   {
> @@ -33,6 +34,16 @@ static void fru_init_complete(bool success)
>   	fru_ready = true;
>   }
>   
> +int pldm_fru_get_bmc_version(void *bv)


That's a dangerous API as we don't know what has been allocated and 
we're going to write over it an unknown size. Should at the minimum take 
an extra parameter specifying the buffer size.


> +{
> +	if (bmc_version == NULL)
> +		return OPAL_HARDWARE;
> +
> +	memcpy(bv, bmc_version, strlen(bmc_version) + 1); > +
> +	return OPAL_SUCCESS;
> +}
> +
>   static int get_fru_record_table_req(void **record_table_data,
>   				    size_t *record_table_length)
>   {
> @@ -115,6 +126,55 @@ static int get_fru_record_table_req(void **record_table_data,
>   	return OPAL_SUCCESS;
>   }
>   
> +int pldm_fru_dt_add_bmc_version(void)
> +{
> +	struct pldm_fru_record_data_format *data;
> +	struct pldm_fru_record_tlv *tlv;
> +	struct dt_node *dt_fw_version;
> +	uint8_t *record_table;
> +	size_t record_size;
> +
> +	dt_fw_version = dt_find_by_name(dt_root, "ibm,firmware-versions");
> +	if (!dt_fw_version)
> +		return OPAL_SUCCESS;
> +
> +	/* retrieve the bmc information with
> +	 * "FRU Record Set Identifier": 1,
> +	 * "FRU Record Type": "General(1)"
> +	 * "FRU Field Type": Version
> +	 *
> +	 * we can not know size of the record table got by options
> +	 * in advance, but it must be less than the source table. So
> +	 * it's safe to use sizeof the source table.
> +	 */
> +	record_table = zalloc(fru_record_length);


We need to verify that the init went fine and 
fru_record_table/fru_record_length are defined. It's a general pattern 
in all those functions with static data. It's the only time I've seen 
where you don't verify it first, but it's probably worth double-checking 
all of them.


> +	record_size = fru_record_length;
> +	get_fru_record_by_option(
> +			fru_record_table,
> +			fru_record_length - PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES,


PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES was already deducted in 
decode_get_fru_record_table_resp(), so fru_record_length should be the 
correct length. At least, that's what I understand :)


> +			record_table,
> +			&record_size,
> +			1,
> +			PLDM_FRU_RECORD_TYPE_GENERAL,
> +			PLDM_FRU_FIELD_TYPE_VERSION);
> +
> +	/* get tlv value */


We need to check 'record_size' and make sure we have (at least) one 
entry in the record table.

   Fred