[PATCH phosphor-host-ipmid v4] Whitelist IPMI commands based on Restricted mode

[Patrick Williams]

On Thu, Jun 16, 2016 at 06:40:41AM -0500, OpenBMC Patches wrote:
> From: tomjose <tomjoseph at in.ibm.com>
> 
> diff --git a/Makefile b/Makefile
> index 7a3b6af..fdae66c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -38,6 +38,7 @@ CFLAGS += -Wall -Wno-unused-result
>  INC_FLAG += $(shell pkg-config --cflags --libs libsystemd) -I. -O2
>  LIB_FLAG += $(shell pkg-config  --libs libsystemd) -rdynamic
>  IPMID_PATH ?= -DHOST_IPMI_LIB_PATH=\"/usr/lib/host-ipmid/\"
> +WHITELIST_PATH?= -DIPMI_WHITELIST_CONF=\"/usr/share/host-ipmid/whitelist.conf\"

I am surprised to see a runtime parsing component here.  I thought we
were going to pass the configuration file to the build and generate a
static set for the whitelist.  Are there pros to this solution?  A
build-time static set would be smaller / faster.

> @@ -157,6 +171,21 @@ ipmi_ret_t ipmi_netfn_router(ipmi_netfn_t netfn, ipmi_cmd_t cmd, ipmi_request_t
>      // Extract the map data onto appropriate containers
>      auto handler_and_context = iter->second;
>  
> +    // If restricted mode is true and command is not whitelisted, don't
> +    // execute the command
> +    if(restricted_mode)
> +    {
> +        auto iter = g_ipmi_whitelist.find(std::make_pair(netfn, cmd));
> +        if(iter == g_ipmi_whitelist.end())

Prefer 'g_ipmi_whitelist.count(ipmi_fn_cmd_t(netfn, cmd)) == 0'.

Reasons:

1. Counts on sets are guaranteed to return 0 or 1 and has the same
   performance as 'find'.

2. Calling the constructor of the underlying set-member ensures there is
   not a conversion cast from pair<X> to pair<Y>.

3. The call for and comparison with the 'end' iterator is eliminated.

> +        {
> +            printf("Net function:[0x%X], Command:[0x%X] is not whitelisted\n", netfn, cmd);
> +            rc = IPMI_CC_INSUFFICIENT_PRIVILEGE;
> +            memcpy(response, &rc, IPMI_CC_LEN);

A little odd pattern for us to use memcpy for one byte, but lucky the
GCC optimizer turns this into a byte-store directly anyhow.

> +void ipmi_populate_whitelist()
> +{
> +    std::ifstream file(IPMI_WHITELIST_CONF);
> +    std::string str;
> +    std::string str_netfn;
> +    std::string str_cmd;
> +    ipmi_netfn_t netfn;
> +    ipmi_cmd_t cmd;
> +    size_t pos = std::string::npos;
> +
> +    // Read the whitelist.conf file and insert the NetFn and Command pair.
> +    // Each entry in the conf file looks like this 0x01:0x02
> +    // ':' character is the separator between NetFn and Command.
> +    while(std::getline(file, str))
> +    {
> +        pos = str.find(':');
> +        if(pos != std::string::npos)
> +        {
> +            str_netfn.assign(str.substr(pos-2,pos-1));
> +            str_cmd.assign(str.substr(pos+3,pos+4));
> +
> +            netfn = strtoul(str_netfn.c_str(), NULL, 16);
> +            cmd = strtoul(str_cmd.c_str(), NULL, 16);
> +
> +            g_ipmi_whitelist.emplace(std::make_pair(netfn, cmd));

Prefer the direct constructor on this one as well.

I know we may change this to build-time but a few comments:

1. Any reason to not support a comment character?
2. Any reason not to use istream >> operators instead of getline and
   then string parsing?
3. Considering the limited utility of the iostream APIs, fscanf might
   be more concise in this case.

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160620/a12b3c4e/attachment.sig>