Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".
spk_xlate isn't used anymore (in line 628), then there is no difference between using cp and buf in VAR_STRING case.
Besides, buf is a const char and those changes remove one uneeded line.
I created the function spk_reset_default_value because it clarifies the code and allows code reusing.
Signed-off-by: Raphael S.Carvalho <[email protected]>
---
drivers/staging/speakup/kobjects.c | 71 ++++++++++++++++++++----------------
1 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index 51bdea3..5c6e77a 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -586,6 +586,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
EXPORT_SYMBOL_GPL(spk_var_show);
/*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+ int *synth_default_value, int idx)
+{
+ struct st_var_header *param;
+
+ if (synth && synth_default_value) {
+ param = spk_var_header_by_name(header_name);
+ if (param) {
+ spk_set_num_var(synth_default_value[idx],
+ param, E_NEW_DEFAULT);
+ spk_set_num_var(0, param, E_DEFAULT);
+ pr_info("%s reset to default value\n", param->name);
+ }
+ }
+}
+
+/*
* This function is called when a user echos a value to one of the
* variable parameters.
*/
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
if (ret == -ERANGE) {
var_data = param->data;
pr_warn("value for %s out of range, expect %d to %d\n",
- attr->attr.name,
+ param->name,
var_data->u.n.low, var_data->u.n.high);
}
+
+ /*
+ * If voice was just changed, we might need to reset our default
+ * pitch and volume.
+ */
+ if (param->var_id == VOICE) {
+ spk_reset_default_value("pitch", synth->default_pitch,
+ value);
+ spk_reset_default_value("vol", synth->default_vol,
+ value);
+ }
break;
case VAR_STRING:
- len = strlen(buf);
- if ((len >= 1) && (buf[len - 1] == '\n'))
+ len = strlen(cp);
+ if ((len >= 1) && (cp[len - 1] == '\n'))
--len;
- if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
- ++buf;
+ if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
+ ++cp;
len -= 2;
}
- cp = (char *) buf;
cp[len] = '\0';
- ret = spk_set_string_var(buf, param, len);
+ ret = spk_set_string_var(cp, param, len);
if (ret == -E2BIG)
pr_warn("value too long for %s\n",
- attr->attr.name);
+ param->name);
break;
default:
pr_warn("%s unknown type %d\n",
param->name, (int)param->var_type);
break;
- }
- /*
- * If voice was just changed, we might need to reset our default
- * pitch and volume.
- */
- if (strcmp(attr->attr.name, "voice") == 0) {
- if (synth && synth->default_pitch) {
- param = spk_var_header_by_name("pitch");
- if (param) {
- spk_set_num_var(synth->default_pitch[value],
- param, E_NEW_DEFAULT);
- spk_set_num_var(0, param, E_DEFAULT);
- }
- }
- if (synth && synth->default_vol) {
- param = spk_var_header_by_name("vol");
- if (param) {
- spk_set_num_var(synth->default_vol[value],
- param, E_NEW_DEFAULT);
- spk_set_num_var(0, param, E_DEFAULT);
- }
- }
- }
+ }
spin_unlock_irqrestore(&speakup_info.spinlock, flags);
if (ret == -ERESTART)
- pr_info("%s reset to default value\n", attr->attr.name);
+ pr_info("%s reset to default value\n", param->name);
return count;
}
EXPORT_SYMBOL_GPL(spk_var_store);
--
1.7.2.5
"Raphael S.Carvalho" <[email protected]> writes:
> + /*
> + * If voice was just changed, we might need to reset our default
> + * pitch and volume.
> + */
> + if (param->var_id == VOICE) {
> + spk_reset_default_value("pitch", synth->default_pitch,
> + value);
> + spk_reset_default_value("vol", synth->default_vol,
> + value);
There's an "invalid read" bug here. You didn't introduce it; it has
been there all along. It's possible that value contains a value that is
out of range, in which case, the spk_reset_default_value calls could
fetch invalid data. The value of ret should be sufficient for
determining whether value is in range, so I'd change the condition of
the if statement to this:
if (param->var_id == VOICE && ret != -ERANGE) {
Or possibly better:
if (param->var_id == VOICE && ret == 0) {
I'd say please resend with that fix, or if not, I can send a one-line
patch to be applied after yours.
-- Chris
Raphael S Carvalho <[email protected]> writes:
> Wouldn't the following code (right before the statement: if
> (param->var_id == VOICE))
> check if value is out of range?
>
> value = simple_strtol(cp, NULL, 10);
> ret = spk_set_num_var(value, param, len);
> if (ret == -ERANGE) {
> var_data = param->data;
> pr_warn("value for %s out of range, expect %d to %d\n",
> param->name,
> var_data->u.n.low, var_data->u.n.high);
> }
That code prints an error message if the value is out of range. Also,
since spk_set_num_var returns -ERANGE, we know that spk_set_num_var
didn't set anything.
But we use value again later in the function, if param->var_id ==
VOICE. In that second use, we don't check to see if value is in range.
So if I set voice to something nonsensical, like 234567, an error
message will be printed, but the calls in the body of the if statement
will use the nonsense value, reading data from an invalid location.
It seems that there's another bug lurking in this code.
If we try to set voice to default, spk_set_num_var returns -ERESTART.
In this case, we shouldn't use value at all when setting the pitch and volume.
"value" is meaningless, regardless of what it contains.
We should use the value of the default voice as the index instead. So
the following should be correct, and you can ignore what I suggested earlier.
if (param->var_id == VOICE && (ret == 0 || ret == -ERESTART)) {
if (ret == -ERESTART)
value = param->data.u.n.default_val;
spk_reset_default_value("pitch", synth->default_pitch,
value);
spk_reset_default_value("vol", synth->default_vol,
value);
}
-- Chris
Good eye for spotting the memory corruption bug!
This is a bug fix, so the fix should go in a separate patch and not
merged with a code cleanup patch. Ordinary users can trigger this so
it's a security bug and separating it out is extra important.
The checking in spk_set_num_var() is not sufficient as well. If we use
E_INC then we can hit an integer overflow bug:
drivers/staging/speakup/varhandlers.c
198 if (how == E_SET)
199 val = input;
200 else
201 val = var_data->u.n.value;
202 if (how == E_INC)
203 val += input;
"input" comes from the user. This addition can overflow so that input
is a very high number and now "val" is a low enough number.
204 else if (how == E_DEC)
205 val -= input;
206 if (val < var_data->u.n.low || val > var_data->u.n.high)
207 return -ERANGE;
"val" is valid, but "input" is not valid. We use "input" in the caller
function as the index to an array.
208 }
I guess that's simple enough to fix but why is the caller using "input"
instead of "val"?
regards,
dan carpenter
Dan Carpenter <[email protected]> writes:
> Good eye for spotting the memory corruption bug!
>
> This is a bug fix, so the fix should go in a separate patch and not
> merged with a code cleanup patch. Ordinary users can trigger this so
> it's a security bug and separating it out is extra important.
Ok. I just sent up a patch to the driverdev list. I missed a few
of the Cc's that were on this thread, though.
Also, it will conflict with Raphael's cleanup.
> The checking in spk_set_num_var() is not sufficient as well. If we use
> E_INC then we can hit an integer overflow bug:
Good catch. In fact, we shouldn't be using input at all! Instead, we
need to use the value of the voice parameter after it was changed. That
will be a valid index into the two tables. My patch does so.
-- Chris
On Tue, Sep 10, 2013 at 06:29:39PM -0700, Chris Brannon wrote:
> Ok. I just sent up a patch to the driverdev list. I missed a few
> of the Cc's that were on this thread, though.
> Also, it will conflict with Raphael's cleanup.
You're missing Raphael's CC in particular...
Really, Raphael's patch arrived first. No one seems to have any
objections to his patch. Normally apply it first and then apply this
one. If we decide to push this one to -stable then we would redo it (in
other words push the one you just sent).
Of course, the merge window is open right now so nothing is going to be
applied for a couple weeks.
regards,
dan carpenter
Raphael S.Carvalho, le Mon 02 Sep 2013 19:20:18 -0300, a ?crit :
> Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
> I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".
>
> spk_xlate isn't used anymore (in line 628), then there is no difference between using cp and buf in VAR_STRING case.
> Besides, buf is a const char and those changes remove one uneeded line.
>
> I created the function spk_reset_default_value because it clarifies the code and allows code reusing.
>
> Signed-off-by: Raphael S.Carvalho <[email protected]>
Acked-by: Samuel Thibault <[email protected]>
> ---
> drivers/staging/speakup/kobjects.c | 71 ++++++++++++++++++++----------------
> 1 files changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index 51bdea3..5c6e77a 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -586,6 +586,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
> EXPORT_SYMBOL_GPL(spk_var_show);
>
> /*
> + * Used to reset either default_pitch or default_vol.
> + */
> +static inline void spk_reset_default_value(char *header_name,
> + int *synth_default_value, int idx)
> +{
> + struct st_var_header *param;
> +
> + if (synth && synth_default_value) {
> + param = spk_var_header_by_name(header_name);
> + if (param) {
> + spk_set_num_var(synth_default_value[idx],
> + param, E_NEW_DEFAULT);
> + spk_set_num_var(0, param, E_DEFAULT);
> + pr_info("%s reset to default value\n", param->name);
> + }
> + }
> +}
> +
> +/*
> * This function is called when a user echos a value to one of the
> * variable parameters.
> */
> @@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
> if (ret == -ERANGE) {
> var_data = param->data;
> pr_warn("value for %s out of range, expect %d to %d\n",
> - attr->attr.name,
> + param->name,
> var_data->u.n.low, var_data->u.n.high);
> }
> +
> + /*
> + * If voice was just changed, we might need to reset our default
> + * pitch and volume.
> + */
> + if (param->var_id == VOICE) {
> + spk_reset_default_value("pitch", synth->default_pitch,
> + value);
> + spk_reset_default_value("vol", synth->default_vol,
> + value);
> + }
> break;
> case VAR_STRING:
> - len = strlen(buf);
> - if ((len >= 1) && (buf[len - 1] == '\n'))
> + len = strlen(cp);
> + if ((len >= 1) && (cp[len - 1] == '\n'))
> --len;
> - if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
> - ++buf;
> + if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
> + ++cp;
> len -= 2;
> }
> - cp = (char *) buf;
> cp[len] = '\0';
> - ret = spk_set_string_var(buf, param, len);
> + ret = spk_set_string_var(cp, param, len);
> if (ret == -E2BIG)
> pr_warn("value too long for %s\n",
> - attr->attr.name);
> + param->name);
> break;
> default:
> pr_warn("%s unknown type %d\n",
> param->name, (int)param->var_type);
> break;
> - }
> - /*
> - * If voice was just changed, we might need to reset our default
> - * pitch and volume.
> - */
> - if (strcmp(attr->attr.name, "voice") == 0) {
> - if (synth && synth->default_pitch) {
> - param = spk_var_header_by_name("pitch");
> - if (param) {
> - spk_set_num_var(synth->default_pitch[value],
> - param, E_NEW_DEFAULT);
> - spk_set_num_var(0, param, E_DEFAULT);
> - }
> - }
> - if (synth && synth->default_vol) {
> - param = spk_var_header_by_name("vol");
> - if (param) {
> - spk_set_num_var(synth->default_vol[value],
> - param, E_NEW_DEFAULT);
> - spk_set_num_var(0, param, E_DEFAULT);
> - }
> - }
> - }
> + }
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
>
> if (ret == -ERESTART)
> - pr_info("%s reset to default value\n", attr->attr.name);
> + pr_info("%s reset to default value\n", param->name);
> return count;
> }
> EXPORT_SYMBOL_GPL(spk_var_store);
> --
> 1.7.2.5
>
--
Samuel
AUTHOR
FvwmM4 is the result of a random bit mutation on a hard
disk, presumably a result of a cosmic-ray or some such
thing.
(extrait de la page de man de FvwmM4)