Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ComponentComparisonStamp was misinterpreted #17

Open
Peng85282 opened this issue Dec 24, 2024 · 4 comments · May be fixed by #18
Open

ComponentComparisonStamp was misinterpreted #17

Peng85282 opened this issue Dec 24, 2024 · 4 comments · May be fixed by #18

Comments

@Peng85282
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

Peng85282 commented Dec 24, 2024 •
edited
Loading

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

@Peng85282
Copy link
Author

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.

@Peng85282 Peng85282 linked a pull request Dec 24, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Check invalid comp_comparison_stamp Peng85282/libpldm
2 participants
@williamspatrick @Peng85282