linux-kernel - Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 08 Sep 2013 23:02:57 -0700
From:	Chris Brannon <chris@...-brannons.com>
To:	Raphael S Carvalho <raphael.scarv@...il.com>
Cc:	William Hubbs <w.d.hubbs@...il.com>, Kirk Reiser <kirk@...sers.ca>,
	Samuel Thibault <samuel.thibault@...-lyon.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	speakup@...ille.uwo.ca, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

Raphael S Carvalho <raphael.scarv@...il.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/