ComponentComparisonStamp was misinterpreted

[Peng85282]

DSP0267 > Table 6 - Component Image Information > ComponentComparisonStamp

The spec says, "When ComponentOptions bit 1 is not set, this field should use the value of 0xFFFFFFFF." "SHOULD" is a recommendation rather than a requirement (DSP0267 Section 3, Terms and definitions). Therefore, the current implementation is incorrect.

However, "When ComponentOptions bit 1 is set, this field shall contain a FD or downstream device vendor selected value to use as a comparison value in determining if a firmware component is down-level or up-level." Since 0xFFFFFFFF is defined as PLDM_FWUP_INVALID_COMPONENT_COMPARISON_TIMESTAMP, it shall not be used when the bit 1 is set.

[williamspatrick]

Do you have example code that is broken and/or an explanation of the bad behavior as a result of this?

Is it just your last sentence that describes the problem?

[Peng85282]

The current code, src/dsp/firmware_update.c at line 641, is
if ((pldm_comp_image_info->comp_options.bits.bit1 == false && pldm_comp_image_info->comp_comparison_stamp != PLDM_FWUP_INVALID_COMPONENT_COMPARISON_TIMESTAMP)) { return PLDM_ERROR_INVALID_DATA; }

It should be
if ((pldm_comp_image_info->comp_options.bits.bit1 == true && pldm_comp_image_info->comp_comparison_stamp == PLDM_FWUP_INVALID_COMPONENT_COMPARISON_TIMESTAMP)) { return PLDM_ERROR_INVALID_DATA; }

According to the spec, when comp_options.bits.bit is not set (i.e. false), comp+comparison_stamp should be simply ignored, even though a value 0xFFFFFFFF is recommended.

I've forked the code and will create a pull request soon.

[Peng85282]

I've created a pull request #18 , but don't know how to link it to this issue.

[Peng85282]

Do you have example code that is broken and/or an explanation of the bad behavior as a result of this?

Is it just your last sentence that describes the problem?

I noticed this issue as the product I'm working with has not to use the timestamp (bit1==false) and the timestamp == 0x00000000. Error was returned by this code. I read the specification and believe this should be allowed.

[amboar]

I'm not convinced that if the corresponding options bit is set that the stamp must not be 0xffffffff.

[Peng85282]

I'm not convinced that if the corresponding options bit is set that the stamp must not be 0xffffffff.

Well. Two issues here.

1. When bit1 is 0, the spec says, "When ComponentOptions bit 1 is not set, this field should use the value of 0xFFFFFFFF." 0xFFFFFFFF is a highly recommended value, but not a hard requirement. Other values are allowed per spec. In general, bit1==false means the field comp_comparison_stamp is not used and it can be simply ignored. Any added restrictions are implementation choice, which could be valid still, but the spec doesn't care.

2. When bit1 is 1, the spec says, "When ComponentOptions bit 1 is set, this field shall contain a FD or downstream device vendor selected value to use as a comparison value in determining if a firmware component is down-level or up-level." The spec doesn't put any recommendations/restrictions to this field, and from the spec point of view, I agree with you that 0xFFFFFFFF could be a valid value.
   However, the implementation defines 0xFFFFFFFF as PLDM_FWUP_INVALID_COMPONENT_COMPARISON_TIMESTAMP. Typically, -1 (i.e. 0xFFFFFFFF for signed int32) means an invalid FD as well. When bit1==true, it's looking for a valid FD or timestamp to do comparison. An invalid FD or timestamp makes no sense here. That is, the current implementation chooses 0xFFFFFFFF to be invalid.

@amboar Sorry that I didn't check back sooner. I wish I made myself clear this time. Please let me know your thoughts. Thanks!

[amboar]

@Peng85282 I think you just want to reduce the strength of the current implementation, not add additional constraints. Can you address the concern in #18 and send an updated patch?

[Peng85282]

@amboar Just to make sure I understand you correctly.

We are to take care only the issue 1 to ignore the comp_comparison_stamp when (bit1==false).

When (bit1==true), we still allow comp_comparison_stamp to be 0xFFFFFFFF (PLDM_FWUP_INVALID_COMPONENT_COMPARISON_TIMESTAMP) as a downstream device vendor selected value.

So, as a result, comp_comparison_stamp could be any 32-bit value, regardless bit1 is set or not.

[Peng85282]

@amboar Sorry for the delay. As suggested, I removed entire comp_comparison_stamp check because

1. when ComponentOptions bit 1 is not set, comp_comparison_stamp is not used and it would be ignored anyway
2. when ComponentOptions bit 1 is set, comp_comparison_stamp would accept any data as a downstream device vendor selected value

[amboar]

@Peng85282 no worries. Regarding PR #18 , it's still the case that the changes need to be submitted through OpenBMC's Gerrit instance. Can you please follow the instructions in the PR?

[Peng85282]

@amboar I understand changes have to be submitted through Gerrit. However, the paperwork never went through. I added PR here anyway so everyone can see the proposed changes, and hopefully someone can help me follow the process and complete the change submission.

[amboar]

However, the paperwork never went through.

Can you describe where you got to in the process? If it was on the project admin side, I might be able to find where it got stuck and sort it out for you.