public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
@ 2013-06-22  1:54 Raphael S. Carvalho
  2013-06-30  8:11 ` Samuel Thibault
  2013-07-23 21:39 ` Greg Kroah-Hartman
  0 siblings, 2 replies; 10+ messages in thread
From: Raphael S. Carvalho @ 2013-06-22  1:54 UTC (permalink / raw)
  To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault,
	Greg Kroah-Hartman, Andy Shevchenko, Andrew Morton, Lijo Antony
  Cc: speakup, devel, linux-kernel, Raphael S. Carvalho

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 608), 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 <raphael.scarv@gmail.com>
---
 drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index 943b6c1..4660ce3 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -585,6 +585,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);
-			}
-		}
-	}
+		break;
+	}
 	spk_unlock(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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-06-22  1:54 [PATCH 1/1] staging/speakup/kobjects.c: Code improvement Raphael S. Carvalho
@ 2013-06-30  8:11 ` Samuel Thibault
  2013-07-23 21:39 ` Greg Kroah-Hartman
  1 sibling, 0 replies; 10+ messages in thread
From: Samuel Thibault @ 2013-06-30  8:11 UTC (permalink / raw)
  To: Raphael S. Carvalho
  Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman,
	Andy Shevchenko, Andrew Morton, Lijo Antony, speakup, devel,
	linux-kernel

Raphael S. Carvalho, le Fri 21 Jun 2013 22:54:40 -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 608), 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 <raphael.scarv@gmail.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
>  1 files changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index 943b6c1..4660ce3 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -585,6 +585,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);
> -			}
> -		}
> -	}
> +		break;
> +	}
>  	spk_unlock(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
$ du temp.iso 
2,0T    temp.iso
$ ls temp.iso -l
-r-xr-xr-x    1 samy     thibault      16E 2003-03-22 14:44 temp.iso*
 -+- je vous dirai pas la marque de mon disque dur, na :p -+- 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-06-22  1:54 [PATCH 1/1] staging/speakup/kobjects.c: Code improvement Raphael S. Carvalho
  2013-06-30  8:11 ` Samuel Thibault
@ 2013-07-23 21:39 ` Greg Kroah-Hartman
  1 sibling, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-23 21:39 UTC (permalink / raw)
  To: Raphael S. Carvalho
  Cc: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault,
	Andy Shevchenko, Andrew Morton, Lijo Antony, speakup, devel,
	linux-kernel

On Fri, Jun 21, 2013 at 10:54:40PM -0300, Raphael S. Carvalho wrote:
> 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 608), 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 <raphael.scarv@gmail.com>
> Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
>  1 files changed, 40 insertions(+), 33 deletions(-)

This patch no longer applies to my tree, can you please refresh it
against linux-next and resend, keeping Samuel's ack on it?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
@ 2013-09-02 22:20 Raphael S.Carvalho
  2013-09-09  3:56 ` Chris Brannon
  2013-09-11 21:01 ` Samuel Thibault
  0 siblings, 2 replies; 10+ messages in thread
From: Raphael S.Carvalho @ 2013-09-02 22:20 UTC (permalink / raw)
  To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault,
	Greg Kroah-Hartman, Andy Shevchenko, Andrew Morton
  Cc: speakup, devel, linux-kernel, Raphael S.Carvalho

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 <raphael.scarv@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-09-02 22:20 Raphael S.Carvalho
@ 2013-09-09  3:56 ` Chris Brannon
       [not found]   ` <CACz=WeeE+jxdaxpR_amF60deDcEy8nChYYfY3UA3CNfhmOB8Gw@mail.gmail.com>
  2013-09-11 21:01 ` Samuel Thibault
  1 sibling, 1 reply; 10+ messages in thread
From: Chris Brannon @ 2013-09-09  3:56 UTC (permalink / raw)
  To: Raphael S.Carvalho
  Cc: William Hubbs, Kirk Reiser, Samuel Thibault, Greg Kroah-Hartman,
	Andy Shevchenko, Andrew Morton, speakup, devel, linux-kernel

"Raphael S.Carvalho" <raphael.scarv@gmail.com> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
       [not found]   ` <CACz=WeeE+jxdaxpR_amF60deDcEy8nChYYfY3UA3CNfhmOB8Gw@mail.gmail.com>
@ 2013-09-09  6:02     ` Chris Brannon
  2013-09-10 23:12       ` Dan Carpenter
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Brannon @ 2013-09-09  6:02 UTC (permalink / raw)
  To: Raphael S Carvalho
  Cc: William Hubbs, Kirk Reiser, Samuel Thibault, Greg Kroah-Hartman,
	Andy Shevchenko, Andrew Morton, speakup, devel, linux-kernel

Raphael S Carvalho <raphael.scarv@gmail.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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-09-09  6:02     ` Chris Brannon
@ 2013-09-10 23:12       ` Dan Carpenter
  2013-09-11  1:29         ` Chris Brannon
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2013-09-10 23:12 UTC (permalink / raw)
  To: Chris Brannon
  Cc: Raphael S Carvalho, devel, Kirk Reiser, speakup,
	Greg Kroah-Hartman, linux-kernel, Samuel Thibault, Andrew Morton,
	Andy Shevchenko

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-09-10 23:12       ` Dan Carpenter
@ 2013-09-11  1:29         ` Chris Brannon
  2013-09-11  7:59           ` Dan Carpenter
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Brannon @ 2013-09-11  1:29 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Raphael S Carvalho, devel, Kirk Reiser, speakup,
	Greg Kroah-Hartman, linux-kernel, Samuel Thibault, Andrew Morton,
	Andy Shevchenko

Dan Carpenter <dan.carpenter@oracle.com> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-09-11  1:29         ` Chris Brannon
@ 2013-09-11  7:59           ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2013-09-11  7:59 UTC (permalink / raw)
  To: Chris Brannon
  Cc: devel, Kirk Reiser, Raphael S Carvalho, speakup,
	Greg Kroah-Hartman, linux-kernel, Samuel Thibault, Andrew Morton,
	Andy Shevchenko

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
  2013-09-02 22:20 Raphael S.Carvalho
  2013-09-09  3:56 ` Chris Brannon
@ 2013-09-11 21:01 ` Samuel Thibault
  1 sibling, 0 replies; 10+ messages in thread
From: Samuel Thibault @ 2013-09-11 21:01 UTC (permalink / raw)
  To: Raphael S.Carvalho
  Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman,
	Andy Shevchenko, Andrew Morton, speakup, devel, linux-kernel

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 <raphael.scarv@gmail.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  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)

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-09-11 21:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-22  1:54 [PATCH 1/1] staging/speakup/kobjects.c: Code improvement Raphael S. Carvalho
2013-06-30  8:11 ` Samuel Thibault
2013-07-23 21:39 ` Greg Kroah-Hartman
  -- strict thread matches above, loose matches on Subject: below --
2013-09-02 22:20 Raphael S.Carvalho
2013-09-09  3:56 ` Chris Brannon
     [not found]   ` <CACz=WeeE+jxdaxpR_amF60deDcEy8nChYYfY3UA3CNfhmOB8Gw@mail.gmail.com>
2013-09-09  6:02     ` Chris Brannon
2013-09-10 23:12       ` Dan Carpenter
2013-09-11  1:29         ` Chris Brannon
2013-09-11  7:59           ` Dan Carpenter
2013-09-11 21:01 ` Samuel Thibault

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox