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.

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

libpldm/platform: Using unions for variable-length fields is incompatible with DSP0248 #29

Open
FHomps opened this issue Feb 4, 2022 · 1 comment

Comments

@FHomps
Copy link

FHomps commented Feb 4, 2022

https://github.com/openbmc/pldm/blob/ef773059fdead2135c96c4a4c3520e4752012ef0/libpldm/platform.h#L426:L455

Following DMTF DSP0248-1.2.1 (and earlier), some PDR fields are of variable length depending on value types specified earlier in the PDR. For example, in section 28.11, table 87:
image

This maxSettable field (and others) should be of a length determined by the effecterDataSize field present earlier in the PDR:
image

Using unions for such a purpose is not possible, as unions always have the size of their biggest member; GCC's __attribute__((packed)) does not change this. This effectively makes all PDRs following this architecture incompatible with the DMTF specification.

@FHomps
Copy link
Author

FHomps commented Feb 9, 2022

I thought about trying to fix this transparently by adding PDR type checks to encode_get_pdr_resp() and decode_get_pdr_resp(), and copying the PDR structure into the PLDM message piece by piece, removing the unwanted padding when needed. There would thus be two PDR representations in the library: one padded out using unions, and a temporary, spec-compliant byte representation used only within the scope of those functions.

However, this will not work, due to caller ownership of the message (for encode_get_pdr_resp()) and of the record data (for decode_get_pdr_resp()) in the current API. The client needs to initialize both those structures and is expected to know their size before calling the functions.

Fixing this issue will thus require changes to the API. I see three possible options to solve this:

  1. Completely rework the PDR system to use getters and setters / dynamic pointer offsets for member access, with underlying opaque structures of runtime sizes representing the PDRs in a spec-compliant manner
  2. Grant ownership of the problematic variable size structures to the encode and decode functions, requiring the client to free them afterwards
  3. Create a compute_pdr_transmission_size() function (or equivalent) to be called before the encode / decode functions. The record data / message size would then depend on the returned dynamic size, allowing the above-mentioned solution to be implemented.

I am personally very much against option 2, and have a preference for option 3. However, all three do break the current API or current usage of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant