Requester: Don't drop messages that don't match the instance id #8
Comments
RashmicaG
pushed a commit
to RashmicaG/libpldm
that referenced
this issue
Feb 14, 2023
Adding new bootprogress state which indicate system firmware or BIOS is starting the operating system at the point of time. This new bootprogress state is taken from PLDM stateset(DSP0249) design spec and stateset id is 196. Signed-off-by: Kamalkumar Patel <[email protected]>
RashmicaG
added a commit
to RashmicaG/libpldm
that referenced
this issue
Mar 23, 2023
This patch has two goals: (1) enable consumers to send PLDM messages over different transports and (2) do this in a way that allows us to move towards a cleaner and more complete requester API. The sole transport option of MCTP via the userspace -demux-daemon is being deprecated. New transports are being added: MCTP via the el (AF_MCTP) and eventually nc-si. The requester API needs updating to support multiple transports, as well as not have MCTP specific ils in the API. To avoid a flag day, the current API has been rewritten terms of the new API. The current API operate at the transport level - they don't ement all of the behaviour necessary for a requester. As such, the new API functions to send/recv a message have the prefix pldm_transport, er than pldm_requester. Given the level that these API are operating these only send and receive a PLDM message. Any additional logic as looking for a response with a particular instance ID belongs at the requester abstraction level. Some of the missing behaviours to fully be a PLDM requester are: assigning instance IDs, request retransmission, implementing outs, and enforcing only one PLDM request to a specific TID at a time. These things are currently implemented in pldmd, meaning any umer other than pldmd using the libpldm "requester" api doesn't get the functionality of a requester and has to implement these things themselves. We would like to eventually move these behaviours into libpldm so the libpldm requester API is actually implmenting what is required e a PLDM requester. This patch starts the move of instance id handling into libpldm. Currently pldmd allocates instance ids, and exposes a DBus call for external apps to get instance ids. It relies on exploiting a viour of mctp-demux to reclaim used ids by monitoring all incoming PLDM responses. When moving to AF_MCTP, PLDM messages are not broadcast all PLDM sockets, and so this method of reclaiming ids won't work automatically. As instance id handling is going into libpldm eventually, we may as well do that now rather than adding an additional step to address issue. So an allocate and free function for instance ids is added in this patch. As this implementation uses file locking, we have the feature of any ids belonging to a process that dies being matically reclaimed into the id pool. The next steps to add in a full requester API, while enabling the of multiple transports look something like this: - convert pldmd to use libpldm instance id API - so all users of PLDM are still using the same instance id allocation method. - convert all consumers of libpldm over to using the new libpldm API, including the instance id functions. - add in the AF_MCTP transport and move consumers over to it. - refactor the encode/decode functions to only encode/decode and not frame the message (ie, remove the instance id from these tions) - add additional requester functionality into libpldm, and have these use the pldm_transport API directly - move consumers over to the new pldm_requester API - remove unused code from pldmd Signed-off-by: Rashmica Gupta <[email protected]> Change-Id: I06e602831f360bbd0efda53d410bfb5080b3100d with '#' will be ignored, and an empty message aborts the commit. # # Date: Fri Dec 9 16:24:45 2022 +1100 # # interactive rebase in progress; onto 0585de9 # Last commands done (4 commands done): # pick 6433752 Adding support for new bootprogress state (openbmc#8) # edit d24bb45 requester: Add new API to support multiple transports # Next commands to do (2 remaining commands): # pick c6fcc60 requester: Add af_mctp transport # pick 6462aa7 prints # You are currently editing a commit while rebasing branch 'new-api' on '0585de9'. # # Changes to be committed: # modified: include/libpldm/base.h # modified: include/libpldm/meson.build # modified: include/libpldm/oem/ibm/libpldm/pdr_oem_ibm.h # modified: include/libpldm/pdr_data.h # modified: include/libpldm/requester/pldm.h # new file: include/libpldm/transport/mctp-demux.h # new file: include/libpldm/transport/transport.h # modified: meson.build # modified: src/meson.build # modified: src/requester/meson.build # modified: src/requester/pldm.c # new file: src/transport/container_of.h # new file: src/transport/mctp-demux.c # new file: src/transport/meson.build # new file: src/transport/transport.c # new file: src/transport/transport.h # include/libpldm/requester/.pldm.h.swo # include/libpldm/requester/.pldm.h.swp # openclient # src/.mctp_defines.h.swp # src/requester/.pldm.c.swo # src/requester/.pldm.c.swp # src/requester/instanceid.c # src/transport/.transport.c.swn # src/transport/.transport.c.swo # src/transport/.transport.c.swp # src/transport/.transport.h.swn # src/transport/.transport.h.swo # src/transport/.transport.h.swp #
bradbishop
pushed a commit
that referenced
this issue
Apr 19, 2023
Fix up an off-by-one error in the test suite. I modified the test to
instead report more sensors than were present in the message buffer and
received the following from ASAN:
```
=================================================================
==297936==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xffffc18300b9 at pc 0xffffa04cae2c bp 0xffffc182fe40 sp 0xffffc182fe58
READ of size 1 at 0xffffc18300b9 thread T0
#0 0xffffa04cae28 in pldm_msgbuf_extract_uint8 ../src/msgbuf/../msgbuf.h:129
#1 0xffffa04cae28 in decode_get_state_sensor_readings_resp ../src/platform.c:804
#2 0x2c0278 in GetStateSensorReadings_testBadDecodeResponse_Test::TestBody() ../tests/libpldm_platform_test.cpp:843
#3 0xffffa03ba0f8 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/lib64/libgtest.so.1.12.1+0x4a0f8)
#4 0xffffa03a6680 in testing::Test::Run() (/lib64/libgtest.so.1.12.1+0x36680)
#5 0xffffa03a688c in testing::TestInfo::Run() (/lib64/libgtest.so.1.12.1+0x3688c)
#6 0xffffa03a6c30 in testing::TestSuite::Run() (/lib64/libgtest.so.1.12.1+0x36c30)
#7 0xffffa03b2dd0 in testing::internal::UnitTestImpl::RunAllTests() (/lib64/libgtest.so.1.12.1+0x42dd0)
#8 0xffffa03b1998 in testing::UnitTest::Run() (/lib64/libgtest.so.1.12.1+0x41998)
#9 0xffffa0400940 in main (/lib64/libgtest_main.so.1.12.1+0x940)
#10 0xffff9f80b584 in __libc_start_call_main (/lib64/libc.so.6+0x2b584)
#11 0xffff9f80b65c in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x2b65c)
#12 0x2b2a6c in _start (/mnt/host/andrew/src/openbmc/libpldm/build/tests/libpldm_platform_test+0x2b2a6c)
Address 0xffffc18300b9 is located in stack of thread T0 at offset 297 in frame
#0 0x2bfe90 in GetStateSensorReadings_testBadDecodeResponse_Test::TestBody() ../tests/libpldm_platform_test.cpp:811
This frame has 14 object(s):
[48, 49) 'retcompletion_code' (line 831)
[64, 65) 'retcomp_sensorCnt' (line 832)
[80, 84) 'rc' (line 819)
[96, 100) '<unknown>'
[112, 116) 'stateField' (line 827)
[128, 132) 'retstateField' (line 833)
[144, 148) '<unknown>'
[160, 168) '<unknown>'
[192, 200) '<unknown>'
[224, 232) '<unknown>'
[256, 264) '<unknown>'
[288, 297) 'responseMsg' (line 815) <== Memory access at offset 297 overflows this variable
[320, 336) 'gtest_ar' (line 822)
[352, 368) 'gtest_ar' (line 847)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
(longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow ../src/msgbuf/../msgbuf.h:129 in pldm_msgbuf_extract_uint8
Shadow bytes around the buggy address:
0x200ff8305fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x200ff8305fd0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
0x200ff8305fe0: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
0x200ff8305ff0: 00 00 f1 f1 f1 f1 f1 f1 01 f2 01 f2 04 f2 f8 f2
0x200ff8306000: 04 f2 04 f2 04 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2
=>0x200ff8306010: f2 f2 00 f2 f2 f2 00[01]f2 f2 f8 f8 f2 f2 00 00
0x200ff8306020: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x200ff8306030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x200ff8306040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x200ff8306050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x200ff8306060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==297936==ABORTING
```
Signed-off-by: Andrew Jeffery <[email protected]>
Change-Id: Ib63254bd345d28924aee47eb230bcbea0c88811a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently the behaviour of the requester is such that we drop any PLDM messages that don't match the instance_id we are looking for. This will probably cause an issue if a consumer wants use the API non-serially, and we should fix this.
The text was updated successfully, but these errors were encountered: