-
Notifications
You must be signed in to change notification settings - Fork 14
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
Define new set of completion codes for libpldm APIs #6
Comments
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
Sorry, but I don't think that's what we want at all. Rather, the problem is that the libpldm APIs are conflating errors that it must signal via its APIs with errors defined by the protocol specifications. These are distinct problems and need to be treated as such. For instance, there's no need for a concept of signalling a failure to allocate dynamic memory at the protocol level, but this is something that's likely necessary for the library. libpldm should not be constrained to returning errors defined in the protocol types through its APIs. We lift that constraint by defining a set of libpldm-specific error codes to return from its APIs that are distinct from the PLDM protocol error codes. |
libpldm uses PLDM spec defined completion codes as return codes for encode/decode APIs.
Eg:- https://github.com/openbmc/libpldm/blob/main/src/firmware_update.c#L622
Which are confusing.
Proposal: Define a new set of return codes as below
Reference to previous discussions:
https://github.com/openbmc/openbmc/wiki/OpenBMC-PMCI-WG#minutes-82420
https://gerrit.openbmc.org/c/openbmc/pldm/+/51443/comments/e7ed8335_4e144ce0
The text was updated successfully, but these errors were encountered: