Re: [PATCH phosphor-host-ipmid] Support for restricted mode for IPMI commands

[Jeremy Kerr <jk@ozlabs.org>]

Hi Tom,

> From: tomjose <tomjoseph@in.ibm.com>

Can you use a full name for the commit, and add some detail to the
commit log?

Also, you may want to check out our contributing guidelines:

  https://github.com/openbmc/docs/blob/master/contributing.md

> @@ -88,6 +89,12 @@ ipmi_ret_t ipmi_app_set_acpi_power_state(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>      *data_len = 0;
>  
>      printf("IPMI SET ACPI STATE Ignoring for now\n");
> +
> +    if(restricted_mode)
> +    {
> +        return IPMI_CC_INSUFFICIENT_PRIVILEGE;
> +    }
> +
>      return rc;
>  }

I don't think this is a maintainable method of implementing restricted
mode, for a couple of reasons:

 - The checks are scattered throughout the codebase. We would have to
   audit every ipmi function to check that restricted mode is
   implemented properly.

 - If we add new commands, we have no way to ensure the whitelist is
   implemented correctly for that command.

We'd be better off implementing the check at a single location, where
the IPMI command is first demultiplexed. This way, we can audit it in a
central location, and have a single list of whitelisted commands.

I'd suggest we have a function, looking something like:

struct {
	ipmi_netfn_t	netfn;
	ipmi_cmd_t	cmd;
} ipmi_whitelist[] = {
	{ NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL },
	....
}

bool ipmi_command_is_allowed(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
	void *data)
{
	int i;

	if (!restricted_mode)
		return true;

	for (i = 0; i < ARRAY_SIZE(ipmi_whitelist); i++) {
		if (netfn == ipmi_whitelist[i].netfn &&
				cmd == ipmi_whitelist[i].cmd)
			return true;
	}

	return false;
}

this would be called from handle_ipmi_command, before we route it to a
handler; if it returns false there, the we return
IPMI_CC_INSUFFICIENT_PRIVILEGE immediately.

This way, we have a whitelist rather than a blacklist, and we can audit
it much more easily.

Regards,


Jeremy