<html><head><META http-equiv="Content-Type" content="text/html;charset=utf-8"></head><body><div id="content" contenteditable="true" spellcheck="true" autocorrect="true" autocapitalize="true" style="font-family: Helvetica;">I'm very confused with this patch. <div><br></div><div>1) sdbus already supports a get and set property so do not create your own</div><div><br></div><div>2) there is a lot of deletes. Feels like this needs some additional eyes. <br><br>Sent from my iPhone using IBM Verse<br><br><hr>On Dec 16, 2015, 4:46:14 PM, "Stewart Smith" <stewart@linux.vnet.ibm.com> wrote:<br><div style="margin-left: 15px;" id="MaaS360PIMSDKOriginalMessageId">OpenBMC Patches <openbmc-patches@stwcx.xyz> writes:<br>> From: shgoupf <shgoupf@cn.ibm.com><br>><br>> 1) Two methods to handle the dbus property set and get:<br>> a) dbus_set_property()<br>> b) dbus_get_property()<br>> 2) The property is stored as a 10 character strings which represents<br>> 5-byte information.<br>> 3) ipmid set method is registered and implemented since petitboot will<br>> use it to clear the boot options.<br>> ---<br>> apphandler.C | 70 ++----------<br>> apphandler.h | 33 +-----<br>> chassishandler.C | 306 ++++++++++++++++++++++++++++++++++++++---------------<br>> chassishandler.h | 20 ++--<br>> ipmid-api.h | 13 +--<br>> ipmid.C | 26 ++---<br>> ipmisensor.C | 35 +-----<br>> sensorhandler.C | 17 ++-<br>> storageaddsel.C | 42 ++++++--<br>> storagehandler.C | 20 +---<br>> transporthandler.C | 2 +<br>> 11 files changed, 304 insertions(+), 280 deletions(-)<br>><br>> diff --git a/apphandler.C b/apphandler.C<br>> index 2c9ce6b..<a dir="ltr" href="tel:6467397" x-apple-data-detectors="true" x-apple-data-detectors-type="telephone" x-apple-data-detectors-result="1">6467397</a> 100644<br>> --- a/apphandler.C<br>> +++ b/apphandler.C<br>> @@ -10,76 +10,20 @@ extern sd_bus *bus;<br>> <br>> void register_netfn_app_functions() __attribute__((constructor));<br>> <br>> -//---------------------------------------------------------------------<br>> -// Called by Host on seeing a SMS_ATN bit set. Return a hardcoded <br>> -// value of 0x2 indicating we need Host read some data.<br>> -//-------------------------------------------------------------------<br>> -ipmi_ret_t ipmi_app_get_msg_flags(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> - ipmi_request_t request, ipmi_response_t response,<br>> - ipmi_data_len_t data_len, ipmi_context_t<br>> context)<br>It is unclear why this is being removed. Is it replaced by something else?<br>> -//-------------------------------------------------------------------<br>> -// Called by Host post response from Get_Message_Flags<br>> -//-------------------------------------------------------------------<br>> ipmi_ret_t ipmi_app_read_event(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> ipmi_request_t request, ipmi_response_t response,<br>> ipmi_data_len_t data_len, ipmi_context_t context)<br>> {<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> - printf("IPMI APP READ EVENT command received\n");<br>> -<br>> - // TODO : For now, this is catering only to the Soft Power Off via OEM SEL<br>> - // mechanism. If we need to make this generically used for some<br>> - // other conditions, then we can take advantage of context pointer.<br>> -<br>> - struct oem_sel_timestamped soft_off = {0};<br>> - *data_len = sizeof(struct oem_sel_timestamped);<br>> -<br>> - // either id[0] -or- id[1] can be filled in. We will use id[0]<br>> - soft_off.id[0] = SEL_OEM_ID_0;<br>> - soft_off.id[1] = SEL_OEM_ID_0;<br>> - soft_off.type = SEL_RECORD_TYPE_OEM;<br>> -<br>> - // Following 3 bytes are from IANA Manufactre_Id field. See below<br>> - soft_off.manuf_id[0]= 0x41;<br>> - soft_off.manuf_id[1]= 0xA7;<br>> - soft_off.manuf_id[2]= 0x00;<br>> -<br>> - // per IPMI spec NetFuntion for OEM<br>> - soft_off.netfun = 0x3A;<br>> -<br>> - // Mechanism to kick start soft shutdown.<br>> - soft_off.cmd = CMD_POWER;<br>> - soft_off.data[0] = SOFT_OFF;<br>> -<br>> - // All '0xFF' since unused.<br>> - memset(&soft_off.data[1], 0xFF, 3);<br>> + *data_len = 0;<br>> <br>> - // Pack the actual response<br>> - memcpy(response, &soft_off, *data_len);<br>> + printf("IPMI APP READ EVENT Ignoring for now\n");<br>> return rc;<br>> +<br>> }<br>Feels like should be a separate patch, I'm unsure as to how this is<br>related to adding get/set methods.<br>> diff --git a/apphandler.h b/apphandler.h<br>> index aa2a55d..35a2b20 100644<br>> --- a/apphandler.h<br>> +++ b/apphandler.h<br>> @@ -1,19 +1,6 @@<br>> #ifndef __HOST_IPMI_APP_HANDLER_H__<br>> #define __HOST_IPMI_APP_HANDLER_H__<br>> <br>> -#include <stdint.h><br>> -<br>> -// These are per skiboot ipmi-sel code<br>> -<br>> -// OEM_SEL type with Timestamp<br>> -#define SEL_OEM_ID_0 0x55<br>> -// SEL type is OEM and -not- general SEL<br>> -#define SEL_RECORD_TYPE_OEM 0xC0<br>> -// Minor command for soft shurdown<br>> -#define SOFT_OFF 0x00<br>> -// Major command for Any kind of power ops<br>> -#define CMD_POWER 0x04<br>> -<br>not sure how the removal of these is related to this patch. Should be a<br>separate one?<br>> // IPMI commands for App net functions.<br>> enum ipmi_netfn_app_cmds<br>> {<br>> @@ -24,27 +11,9 @@ enum ipmi_netfn_app_cmds<br>> IPMI_CMD_RESET_WD = 0x22,<br>> IPMI_CMD_SET_WD = 0x24,<br>> IPMI_CMD_SET_BMC_GLOBAL_ENABLES = 0x2E,<br>> - IPMI_CMD_GET_MSG_FLAGS = 0x31,<br>> IPMI_CMD_READ_EVENT = 0x35,<br>> IPMI_CMD_GET_CAP_BIT = 0x36,<br>> -};<br>same here.<br>> <br>> -// A Mechanism to tell host to shtudown hosts by sending this PEM SEL. Really<br>> -// the only used fields by skiboot are:<br>> -// id[0] / id[1] for ID_0 , ID_1<br>> -// type : SEL_RECORD_TYPE_OEM as standard SELs are ignored by skiboot<br>> -// cmd : CMD_POWER for power functions<br>> -// data[0], specific commands. example Soft power off. power cycle, etc.<br>> -struct oem_sel_timestamped<br>> -{<br>> - /* SEL header */<br>> - uint8_t id[2];<br>> - uint8_t type;<br>> - uint8_t manuf_id[3];<br>> - uint8_t timestamp[4];<br>> - /* OEM SEL data (6 bytes) follows */<br>> - uint8_t netfun;<br>> - uint8_t cmd;<br>> - uint8_t data[4];<br>> };<br>> +<br>> #endif<br>and here.<br>> diff --git a/chassishandler.C b/chassishandler.C<br>> index 1389db9..7694bd5 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -4,10 +4,147 @@<br>> #include <string.h><br>> #include <stdint.h><br>> <br>> -// OpenBMC Chassis Manager dbus framework<br>> -const char *chassis_bus_name = "org.openbmc.control.Chassis";<br>> -const char *chassis_object_name = "/org/openbmc/control/chassis0";<br>> -const char *chassis_intf_name = "org.openbmc.control.Chassis";<br>> +char* uint8_to_char(uint8_t *a, size_t size)<br>> +{<br>> + char* buffer;<br>> + int i;<br>> +<br>> + buffer = (char*)malloc(size * 2 + 1);<br>> + if (!buffer)<br>> + return NULL;<br>> +<br>> + buffer[size * 2] = 0;<br>> + for (i = 0; i < size; i++) {<br>> + uint8_t msb = (a[i] >> 4) & 0xF;<br>> + uint8_t lsb = a[i] & 0xF;<br>> + buffer[2*i] = msb > 9 ? msb + 'A' - 10 : msb + '0';<br>> + buffer[2*i + 1] = lsb > 9 ? lsb + 'A' - 10 : lsb + '0';<br>> + }<br>> +<br>> + return buffer;<br>> +}<br>> +<br>> +uint8_t* char_to_uint8(char *a, size_t size)<br>> +{<br>> + uint8_t* buffer;<br>> + int i;<br>> +<br>> + buffer = (uint8_t*)malloc(size);<br>> + if (!buffer)<br>> + return NULL;<br>> +<br>> + for (i = 0; i < size; i++) {<br>> + uint8_t msb = (uint8_t)(a[2*i] > '9' ? a[2*i] - 'A' + 10 : a[2*i] - '0');<br>> + uint8_t lsb = (uint8_t)(a[2*i+1] > '9' ? a[2*i+1] - 'A' + 10 : a[2*i+1] - '0');<br>> + buffer[i] = ((msb << 4) | (lsb & 0xF)) & 0xFF;<br>> + }<br>> +<br>> + return buffer;<br>> +}<br>What do these functions *do*? The names of uint8_to_char and<br>char_to_uint8 make absolutely zero sense to me as a char is, in fact, 8<br>bits, i.e. a uint8_t, just with a different semantic meaning.<br>Is there some weird IPMI specific format of strings?<br>> -//----------------------------------------------------------------------<br>> -// Chassis Control commands<br>> -//----------------------------------------------------------------------<br>> -ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd, <br>> - ipmi_request_t request, ipmi_response_t response, <br>> - ipmi_data_len_t data_len, ipmi_context_t context)<br>> -{<br>> - // Error from power off.<br>> - int rc = 0;<br>> + char* buf = (char*)malloc(NUM_RETURN_BYTES_OF_GET);<br>> <br>> - // No response for this command.<br>> - *data_len = 0;<br>> + if (reqptr->parameter == 5) // Parameter #5<br>> + {<br>> + dbus_get_property(buf);<br>> + uint8_t* return_value = char_to_uint8(buf, NUM_RETURN_BYTES_OF_GET_USED);<br>> + *data_len = NUM_RETURN_BYTES_OF_GET;<br>> + // TODO: last 3 bytes<br>> + // (NUM_RETURN_BYTES_OF_GET - NUM_RETURN_BYTES_OF_GET_USED) is meanlingless<br>> + memcpy(response, return_value, *data_len);<br>> + free(buf);<br>> + free(return_value);<br>This isn't safe if dbus_get_property fails (which it can). Indeed, if it<br>does, you'll be running a whole bunch of code on unitialised data. If<br>you're lucky you'll segv, if unlucky, exploitable security hole.<br>> -ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd, <br>> +ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd, <br>> ipmi_request_t request, ipmi_response_t response, <br>> ipmi_data_len_t data_len, ipmi_context_t context)<br>> {<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> - *data_len = 0;<br>> <br>> - printf("IPMI GET_SYS_BOOT_OPTIONS\n");<br>> + printf("IPMI SET_SYS_BOOT_OPTIONS\n");<br>> + printf("IPMID set command required return bytes: %i\n", *data_len);<br>> <br>> - get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;<br>> + set_sys_boot_options_t *reqptr = (set_sys_boot_options_t*) request;<br>> <br>> - // TODO Return default values to OPAL until dbus interface is available<br>> + char* output_buf = (char*)malloc(NUM_RETURN_BYTES_OF_SET);<br>This pattern repeats itself a lot - not checking for return value of<br>malloc.<br>Since OpenBMC is going to be running in a relatively constrained<br>resource environment, likely without swap and there's probably a good<br>argument for turning memory overcommit off, you probably want to fail<br>somewhat gracefully - or at the very least assert() and fail in a known<br>way rather than continue ond dereference null pointers.<br>> @@ -141,6 +257,28 @@ void register_netfn_chassis_functions()<br>> printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);<br>> ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);<br>> <br>> - printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);<br>> - ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);<br>> + printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS);<br>> + ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_set_sys_boot_options);<br>> +<br>> + // TODO: Testing for dbus property set/get and related methods.<br>> + printf("----> Start of chassis handler testing.\n");<br>> + set_sys_boot_options_t req = {0x80, 0x10, 0xA2, 0x3B, 0x45, 0x57}; <br>> + char* set_value = uint8_to_char((uint8_t*)(&(req.data1)), 5);<br>> + dbus_set_property(set_value);<br>> + char* buf = (char*)malloc(NUM_RETURN_BYTES_OF_GET_USED * 2 + 1);<br>again, not checking results of dbus_set_property or malloc.<br>> + dbus_get_property(buf);<br>> + uint8_t* get_value = char_to_uint8(buf, NUM_RETURN_BYTES_OF_GET_USED);<br>> + int i;<br>> + printf("buf: %s\n", (char*)buf);<br>> + printf("0x");<br>> + for (i = 0; i < 5; i++) {<br>> + printf("%2x", get_value[i]);<br>> + }<br>> + printf("\n");<br>> + printf("----> End of chassis handler testing.\n");<br>> + free(buf);<br>> + free(set_value);<br>> + free(get_value);<br>> }<br>> +<br>> +<br>> diff --git a/chassishandler.h b/chassishandler.h<br>> index 1a26411..80e40a8 100644<br>> --- a/chassishandler.h<br>> +++ b/chassishandler.h<br>> @@ -1,14 +1,18 @@<br>> #ifndef __HOST_IPMI_CHASSIS_HANDLER_H__<br>> #define __HOST_IPMI_CHASSIS_HANDLER_H__<br>> <br>> -#include <stdint.h><br>> +// TODO: Petitboot requires 8 bytes of response<br>> +// however only 5 of them are used.<br>Is this due to petitboto behaviour quirk or something that's<br>standardized somewhere?<br>what should the unused bytes be set to? 0x00? 0xFF?<br>_______________________________________________<br>openbmc mailing list<br>openbmc@lists.ozlabs.org<br>https://lists.ozlabs.org/listinfo/openbmc<br></stdint.h></stdint.h></string.h></stdint.h></shgoupf@cn.ibm.com></openbmc-patches@stwcx.xyz></div></div></div><BR>
</body></html>