Hey Vince,
> 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.
IMO, this is more optimal. For the average user, if you input (for
example) 99999 and the value set is 159. It will just confuse people.
> 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.
Yup, this is the case.
> I've had some difficulty locating data in the IPMI
> specifications that lay out what the legal values are for each of these
> parameters
They are scattered within Table 26-5 of the spec. Comments inlined:
@@ -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;
This seems like the character accumulate interval. Range is apparently
1-255 b/c 00h is listed as reserved. (I assumed 00h was allowed in
FreeIPMI too, whoops).
/* 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))
0-255 seems fine for the character send threshold.
@@ -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))
0-7 seems right b/c it's a 3 bit field.
@@ -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))
0-255 seems right.
Al
On Tue, 2007-08-07 at 13:40 -0400, Vince Worthington wrote:
> 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:
>
> -------------------------------------------------------------------------
> 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
--
Albert Chu
[EMAIL PROTECTED]
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory
-------------------------------------------------------------------------
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
