SUBJ: proposed patch to validate user-provided sol parameter values
Hello,
It was brought to our attention by a customer that when configuring SOL
communication parameters via the 'ipmitool sol set' facility of ipmitool,
there is no range checking performed on parameters such as
character-send-threshold. This appears to be the case for all parameters
which (in effect) a uint8_t data type to hold the value provided by the
user.
Additionally, with the user-provided value cast to type uint8_t, checking
the value that got set afterwards may be a little confusing. In one
example, attempting to set retry-count to 14 results in a value of 6
actually getting set. While some of this effect in the specific case of
retry-count is due to the bitwise AND with 0x07, it may be similarly
confusing for other parameters. Attempting to character-send-threshold to
99999 results in an actual value of 159 (from memory of my testing) being
set.
I admit that I'm not entirely certain whether the intention of the utility
would be more geared towards taking whatever the user provides as a value
and just working on it "behind the scenes" to send something more valid to
the BMC, which may be more useful in the case of a scripted execution of
the utility -- or if it would be agreed that providing the user a visible
error message (as is done with other serial parameters) would be more
valuable.
The attached patch contains a proposed function to validate the values
provided by the user. The idea with the patch is to be able to specify
(with the third and fourth arguments) a specific minimum and maximum
allowable value, since some of the configurable parameters seem to have
different "legal" ranges of values. If the value supplied by the user
doesn't "fit" in a uint8_t, or is outside the min and max values passed to
the function, an error message similar to that issued by the check
routines for other parameters is printed to the user and a return of -1
occurs.
One additional aspect of this patch in the form presented here is the
change made to setting the retry-count value. The original code performs
a bitwise AND with the user-supplied value. I am assuming this is there
because 7 is the maximum value that you can really set this to. I may be
misunderstanding this somewhat, or there may be a preference to using a
"behind the scenes" manipulation of the value, rather than an error
condition being triggered. I'm curious what the opinion on this idea
would be.
Unfortunately I've had some difficulty locating data in the IPMI
specifications that lay out what the legal values are for each of these
parameters, though I can also imagine there maybe being some variance
between hardware implementations. But with the patch as it is presented,
ipmitool still handles an unfavorable response from the BMC.
Ultimately our customer is asking us to provide a means to do
range-checking on the parameter values supplied to the 'ipmitool sol set'
facility, and return an error if an invalid value is provided. How this
patch does it may or may not be what the ipmitool developers would want,
but thank you for any review or discussion of this proposed patch, and I
appreciate any consideration for the concept that may result. If the idea
is good, but needs to be done differently, please let me know. :)
I'll keep an eye on the mailing list for responses, discussion, etc. and
continue to participate on the list.
Thanks,
Vince Worthington
Patch attached below:
--- ipmitool-1.8.9/include/ipmitool/ipmi_sol.h.check-ranges 2006-03-19 12:59:39.000000000 -0500
+++ ipmitool-1.8.9/include/ipmitool/ipmi_sol.h 2007-08-07 11:27:45.000000000 -0400
@@ -75,6 +75,19 @@ struct activate_payload_rsp {
} __attribute__ ((packed));
+/*
+ * Small function to validate that user-supplied SOL
+ * configuration parameter values we store in uint8_t
+ * data type falls within valid range. With minval
+ * and maxval parameters we can use the same function
+ * to validate parameters that have different ranges
+ * of values.
+ */
+int ipmi_sol_set_param_isvalid_uint8_t( const long *value,
+ const char *param,
+ const uint8_t *minval,
+ const uint8_t *maxval);
+
int ipmi_sol_main(struct ipmi_intf *, int, char **);
int ipmi_get_sol_info(struct ipmi_intf * intf,
uint8_t channel,
--- ipmitool-1.8.9/lib/ipmi_sol.c.check-ranges 2007-01-11 13:36:25.000000000 -0500
+++ ipmitool-1.8.9/lib/ipmi_sol.c 2007-08-07 11:27:45.000000000 -0400
@@ -581,6 +581,37 @@ ipmi_print_sol_info(struct ipmi_intf * i
/*
+ * Small function to validate that user-supplied SOL
+ * configuration parameter values we store in uint8_t
+ * data type falls within valid range. With minval
+ * and maxval parameters we can use the same function
+ * to validate parameters that have different ranges
+ * of values.
+ *
+ * function will return 0 if value is not valid, or
+ * will return 1 if valid.
+ */
+
+
+int ipmi_sol_set_param_isvalid_uint8_t( const long *value,
+ const char *param,
+ const uint8_t *minval,
+ const uint8_t *maxval)
+
+{
+ if ( (strtol(value, NULL, 0) < minval) || (strtol(value, NULL, 0) > maxval))
+ {
+ lprintf(LOG_ERR, "Invalid value %s for parameter %s",
+ value, param);
+ lprintf(LOG_ERR, "Valid values are %d-%d", minval, maxval);
+ return 0;
+ }
+ else
+ return 1;
+}
+
+
+/*
* ipmi_sol_set_param
*
* Set the specified Serial Over LAN value to the specified
@@ -777,7 +808,12 @@ ipmi_sol_set_param(struct ipmi_intf * in
req.msg.data_len = 4;
data[1] = SOL_PARAMETER_CHARACTER_INTERVAL;
- data[2] = (uint8_t)strtol(value, NULL, 0);
+
+ /* validate user-supplied input */
+ if (ipmi_sol_set_param_isvalid_uint8_t(value, param, 0, 255))
+ data[2] = (uint8_t)strtol(value, NULL, 0);
+ else
+ return -1;
/* We need other values to complete the request */
if (ipmi_get_sol_info(intf, channel, ¶ms))
@@ -800,7 +836,12 @@ ipmi_sol_set_param(struct ipmi_intf * in
req.msg.data_len = 4;
data[1] = SOL_PARAMETER_CHARACTER_INTERVAL;
- data[3] = (uint8_t)strtol(value, NULL, 0);
+
+ /* validate user-supplied input */
+ if (ipmi_sol_set_param_isvalid_uint8_t(value, param, 0, 255))
+ data[3] = (uint8_t)strtol(value, NULL, 0);
+ else
+ return -1;
/* We need other values to complete the request */
if (ipmi_get_sol_info(intf, channel, ¶ms))
@@ -823,7 +864,16 @@ ipmi_sol_set_param(struct ipmi_intf * in
req.msg.data_len = 4;
data[1] = SOL_PARAMETER_SOL_RETRY;
- data[2] = (uint8_t)strtol(value, NULL, 0) & 0x07;
+
+ /* validate user input, 7 is max value */
+ if (ipmi_sol_set_param_isvalid_uint8_t(value, param, 0, 7))
+ {
+ data[2] = (uint8_t)strtol(value, NULL, 0);
+ }
+ else
+ {
+ return -1;
+ }
/* We need other values to complete the request */
if (ipmi_get_sol_info(intf, channel, ¶ms))
@@ -846,7 +896,12 @@ ipmi_sol_set_param(struct ipmi_intf * in
req.msg.data_len = 4;
data[1] = SOL_PARAMETER_SOL_RETRY;
- data[3] = (uint8_t)strtol(value, NULL, 0);
+
+ /* validate user-supplied input */
+ if (ipmi_sol_set_param_isvalid_uint8_t(value, param, 0, 255))
+ data[3] = (uint8_t)strtol(value, NULL, 0);
+ else
+ return -1;
/* We need other values to complete the request */
if (ipmi_get_sol_info(intf, channel, ¶ms))-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
