| commit | 8f33a1dcc30df24ad44c4b022273087fd234cb20 | [log] [tgz] |
|---|---|---|
| author | Andrew Jeffery <andrew@codeconstruct.com.au> | Fri Oct 11 18:01:19 2024 +1030 |
| committer | Andrew Jeffery <andrew@codeconstruct.com.au> | Fri Oct 11 19:21:18 2024 +1030 |
| tree | 44518c184badd0c0702e7d362211c5ed9d4d1a95 | |
| parent | 36324f6bea8c5de1e380659bb56aca5e0b8724b4 [diff] |
dsp: pdr: Rework test in pldm_entity_association_pdr_extract()
Testing on IBM's simulator[1] and bisection by Manoj
found that 9e566597d91e ("dsp: pdr: Bound check
pldm_entity_association_pdr_extract()") caused their host boot process
to come unstuck[1]:
[1]: https://gerrit.openbmc.org/c/openbmc/openbmc/+/75130/comments/fc6548ce_78285814
```
26.98655|2024/10/08 10:04:51|ISTEP 6. 5 - host_set_ipl_parms
27.02035|2024/10/08 10:04:51|ISTEP 6. 6 - host_discover_targets
27.31030|Detected new part : 00030000 (Physical:/Sys0/Node0/DIMM0)
27.64652|Detected new part : 00030004 (Physical:/Sys0/Node0/DIMM2)
29.58074|Detected new part : 00050000 (Physical:/Sys0/Node0/Proc0)
43.28986|Detected new part : 00050000 (Physical:/Sys0/Node0/Proc0)
47.49649|HWAS|---------------------------------
47.49650|HWAS|PRESENT>
47.52988|TARG|PROC=80000000
47.53612|TARG|PROC[00]:
47.53616|TARG| CORE=FF000000 DIMM=8800000000000000
47.53620|TARG| CACHE=FF000000 OCMB=C000
47.53632|TARG|PROC[01]:
47.53637|TARG| CORE=00000000 DIMM=0000000000000000
47.53641|TARG| CACHE=00000000 OCMB=0000
47.3653|TARG|PROC[02]:
47.53658|TARG| CORE=00000000 DIMM=0000000000000000
47.53662|TARG| CACHE=00000000 OCMB=0000
47.53674|TARG|PROC[03]:
47.53679|TARG| CORE=00000000 DIMM=0000000000000000
47.53683|TARG| CACHE=00000000 OCMB=0000
47.53738|WAS|---------------------------------
47.56477|devtree|Syncing to BMC
57.36060|Triggering graceful reboot fr Rebooting due to a FRU hot-remove
57.85530|================================================
57.87003|Error reprted by pldm (0x4700) EID 0x90000013
57.88170| Software problem, could not find reboot count effecter PDR.
57.88174| ModuleId 0x3e MOD_RESET_REBOOT_COUNT
57.90019| ReasonCode 0x471a RC_INVALID_EFFECTER_ID
57.90028| UserData1 The total number of PDRs that PDR Manager is aware of. : 0x00000000000001ee
57.90031| UserData2 : 0x0000000000000000
57.90034|------------------------------------------------
57.90038| Callouttype : Procedure Callout
57.90041| Procedure : EPUB_PRC_HB_CODE
57.92782| Priority : SRCI_PRIORITY_HIGH
57.92785|-----------------------------------------------
57.92788| Callout type : Procedure Callout
57.92791| Procedure : EPUB_PRC_SP_COE
57.92794| Priority : SRCI_PRIORITY_HIGH
57.92798|-----------------------------------------------
57.92801| Callout type : Procedure Callout
57.92804| Procedure : EPUB_PRC_HB_CODE
57.92808| Priority : SRCI_PRIORITY_MED
57.92811|------------------------------------------------
57.92816| Hostboot Build ID: hostboot-p11-e7437ab-sha1:997c6a7f/hbicore.bin
57.92819|================================================
59.00252|Soft poweroff requested by the BMC
```
This seems to be the result of the following from the BMC's side:
```
Oct 08 10:04:30 p10bmc pldmd[642]: Instance ID expiry for EID '9' using InstanceID '1'
Oct 08 10:04:30 p10bmc pldmd[642]: Failed to receive response for setEventReceiver command
Oct 08 10:04:32 p10bmc pldmd[642]: sdeventplus: ioCallback: Instance ID 1 for TID 9 was not previously allocated
Oct 08 10:04:32 p10bmc pldmd[642]: sdeventplus: ioCallback: Instance ID 1 for TID 9 was not previously allocated
Oct 08 10:04:32 p10bmc pldmd[642]: sdeventplus: ioCallback: Instance ID 1 for TID 9 was not previously allocated
Oct 08 10:04:32 p10bmc bmcwebd[282]: PAM unable to resolve symbol: pam_sm_acct_mgmt
Oct 08 10:04:33 p10bmc bmcwebd[282]: pam_succeed_if(webserver:auth): requirement "user ingroup redfish" was met by user "root"
Oct 08 10:04:35 p10bmc bmcwebd[282]: PAM unable to resolve symbol: pam_sm_acct_mgmt
Oct 08 10:04:35 p10bmc pldmd[642]: Checking if directory '/usr/share/pldm/pdr' exists
Oct 08 10:04:35 p10bmc pldmd[642]: Checking if directory '/usr/share/pldm/pdr/com.ibm.Hardware.Chassis.Model.Rainier4U' exists
Oct 08 10:04:35 p10bmc pldmd[642]: Failed to create sensor PDR, D-Bus object '/org/freedesktop/UPower/devices/ups_hiddev0' returned error - sd_bus_call: xyz.openbmc_project.Common.Error.ResourceNotFound: The resource is not found.
Oct 08 10:04:35 p10bmc bmcwebd[282]: pam_succeed_if(webserver:auth): requirement "user ingroup redfish" was met by user "root"
Oct 08 10:04:36 p10bmc pldmd[642]: Failed to create sensor PDR, D-Bus object '/xyz/openbmc_project/led/physical/virtual_enc_id' returned error - sd_bus_call: xyz.openbmc_project.Common.Error.ResourceNotFound: The resource is not found.
Oct 08 10:04:36 p10bmc pldmd[642]: Failed to create sensor PDR, D-Bus object '/xyz/openbmc_project/led/physical/virtual_enc_fault' returned error - sd_bus_call: xyz.openbmc_project.Common.Error.ResourceNotFound: The resource is not found.
```
Ultimately the check was trying to satisfy the following from GCC's
analyzer:
```
| 1345 | size_t l_num_entities = entity_association_pdr->num_children + 1;
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (16) ...to here
| 1346 | if (l_num_entities < 2) {
| | ~
| | |
| | (17) following ‘false’ branch (when ‘l_num_entities > 1’)...
|......
| 1354 | pldm_entity *l_entities = calloc(l_num_entities, sizeof(pldm_entity));
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (18) ...to here
| | (19) use of attacker-controlled value ‘(size_t)((int)*((char *)&*pdr + 10).num_children + 1) * 6’ as size without upper-bounds checking
|
```
However, rather than the relationship to pdr_len, the issue was the
confusion from the promotion from uint8_t to size_t. Teach the analyzer
about the UINT8_MAX limit explicitly.
gitlint-ignore: UC1, B1
Fixes: 9e566597d91e ("dsp: pdr: Bound check pldm_entity_association_pdr_extract()")
Change-Id: I0c71ba3b80da2946658a4e6a3add4636752b4e74
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
This is a library which deals with the encoding and decoding of PLDM messages. It should be possible to use this library by projects other than OpenBMC, and hence certain constraints apply to it:
Source files are named according to the PLDM Type, for eg base.[h/c], fru.[h/c], etc.
Given a PLDM command "foo", the library will provide the following API: For the Requester function:
encode_foo_req() - encode a foo request decode_foo_resp() - decode a response to foo
For the Responder function:
decode_foo_req() - decode a foo request encode_foo_resp() - encode a response to foo
The library also provides API to pack and unpack PLDM headers.
Need meson and ninja. Alternatively, source an OpenBMC ARM/x86 SDK.
meson setup builddir && ninja -C builddir
The simplest way of running the tests is as described by the meson man page:
meson setup builddir && meson test -C builddir
libpldmComponents of the library ABI[^1] (loosely, functions) are separated into three categories:
[^1]: "library API + compiler ABI = library ABI"
Applications depending on libpldm should aim to only use functions from the stable category. However, this may not always be possible. What to do when required functions fall into the deprecated or testing categories is outlined below.
Marking a function as stable makes the following promise to users of the library:
We will not remove or change the symbol name, argument count, argument types, return type, or interpretation of relevant values for the function before first marking it as
LIBPLDM_ABI_DEPRECATEDand then subsequently creating a tagged release
Marking a function as stable does not promise that it is free of implementation bugs. It is just a promise that the prototype won't change without notice.
Given this, it is always okay to implement functions marked stable in terms of functions marked testing inside of libpldm. If we remove or change the prototype of a function marked testing the only impact is that we need to fix up any call sites of that function in the same patch.
--- title: libpldm symbol lifecycle --- stateDiagram-v2 direction LR [*] --> Testing: Add Testing --> Testing: Change Testing --> [*]: Remove Testing --> Stable: Stabilise Stable --> Deprecated: Deprecate Deprecated --> [*]: Remove
The ABI of the library produced by the build is controlled using the abi meson option. The following use cases determine how the abi option should be specified:
| Use Case | Meson Configuration |
|---|---|
| Production | -Dabi=deprecated,stable |
| Maintenance | -Dabi=stable |
| Development | -Dabi=deprecated,stable,testing |
Applications and libraries that depend on libpldm can identify how to migrate off of deprecated APIs by constraining the library ABI to the stable category. This will force the compiler identify any call-sites that try to link against deprecated symbols.
Applications and libraries often require functionality that doesn't yet exist in libpldm. The work is thus in two parts:
libpldmlibpldm in the dependent application or libraryAdding APIs to a library is a difficult task. Generally, once an API is exposed in the library's ABI, any changes to the API risk breaking applications already making use of it. To make sure we have more than one shot at getting an API right, all new APIs must first be exposed in the testing category. Concretely:
Patches adding new APIs MUST mark them as testing and MUST NOT mark them as stable.
Three macros are provided through config.h (automatically included for all translation units) to mark functions as testing, stable or deprecated:
LIBPLDM_ABI_TESTINGLIBPLDM_ABI_STABLELIBPLDM_ABI_DEPRECATEDThese annotations go immediately before your function signature:
LIBPLDM_ABI_TESTING pldm_requester_rc_t pldm_transport_send_msg(struct pldm_transport *transport, pldm_tid_t tid, const void *pldm_req_msg, size_t req_msg_len) { ... }
As mentioned above, all new functions must first be added in the testing category (using the LIBPLDM_ABI_TESTING annotation).
To move a function from the testing category to the stable category, its required that patches demonstrating use of the function in a dependent application or library be linked in the commit message of the stabilisation change. We require this to demonstrate that the implementer has considered its use in context before preventing us from making changes to the API.
Meson is broadly used in the OpenBMC ecosystem, the historical home of libpldm. Meson's subprojects are a relatively painless way of managing dependencies for the purpose of developing complex applications and libraries. Use of libpldm as a subproject is both supported and encouraged.
libpldm's ABI can be controlled from a parent project through meson's subproject configuration syntax:
meson setup ... -Dlibpldm:abi=deprecated,stable,testing ...
This will support OEM or vendor-specific functions and semantic information. Following directory structure has to be used:
libpldm |---- include/libpldm | |---- oem/<oem_name> | |----<oem based .h files> |---- src | |---- oem/<oem_name> | |----<oem based .c files> |---- tests | |---- oem/<oem_name> | |----<oem based test files>
<oem_name> - This folder must be created with the name of the OEM/vendor in lower case.
Header files & source files having the oem functionality for the libpldm library should be placed under the respective folder hierarchy as mentioned in the above figure. They must be adhering to the rules mentioned under the libpldm section above.
Once the above is done a meson option has to be created in meson.options with its mapped compiler flag to enable conditional compilation.
For consistency would recommend using "oem-<oem_name>".
The meson.build and the corresponding source file(s) will need to incorporate the logic of adding its mapped compiler flag to allow conditional compilation of the code.