From 40a3c73f21311521a510f2df4883296482707302 Mon Sep 17 00:00:00 2001
From: Pavel Zhigulin <Pavel.Zhigulin@kaspersky.com>
To: William Hubbs <w.d.hubbs@gmail.com>
Cc: Chris Brannon <chris@the-brannons.com>, Kirk Reiser <kirk@reisers.ca>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Cc: speakup@linux-speakup.org, linux-kernel@vger.kernel.org
Cc: lvc-project@linuxtesting.org
Date: Fri, 26 Sep 2025 23:17:03 +0300
Subject: [PATCH] speakup: keyhelp: guard letter_offsets possible out-of-range
 indexing

help_init() builds letter_offsets[] by using the first byte of each
function name as an index via `(start & 31) - 1`. If function_names are
overridden from sysfs (root) with a name starting outside [a–z], the
index underflows or exceeds the array, leading to OOB write.

Function names can be overridden with the following commands as root:

    modprobe speakup_soft
    echo "0 _bad" > /sys/accessibility/speakup/i18n/function_names
    # then press Insert+2 on /dev/tty

This fix checks the first letter in help_init(), and if it is not in the
[a–z] range the function returns an error to the caller. Eventually this
error is propagated to drivers/accessibility/speakup/main.c:2217, which
causes a bleep sound.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: a4efe6fd5dea ("staging: speakup: (coding style) Add spaces around operands (checkpatch checks)")
Signed-off-by: Pavel Zhigulin <Pavel.Zhigulin@kaspersky.com>
---
 drivers/accessibility/speakup/keyhelp.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/accessibility/speakup/keyhelp.c b/drivers/accessibility/speakup/keyhelp.c
index 822ceac83068..df60a8b15a2f 100644
--- a/drivers/accessibility/speakup/keyhelp.c
+++ b/drivers/accessibility/speakup/keyhelp.c
@@ -8,6 +8,7 @@
  */

 #include <linux/keyboard.h>
+#include <linux/ctype.h>
 #include "spk_priv.h"
 #include "speakup.h"

@@ -120,10 +121,20 @@ static int help_init(void)
 	state_tbl = spk_our_keys[0] + SHIFT_TBL_SIZE + 2;
 	for (i = 0; i < num_funcs; i++) {
 		char *cur_funcname = spk_msg_get(MSG_FUNCNAMES_START + i);
+		char first_letter;

-		if (start == *cur_funcname)
+		if (!cur_funcname)
+			return -1;
+
+		first_letter = tolower(*cur_funcname);
+
+		/* Accept only 'a'..'z' to index letter_offsets[] safely */
+		if (first_letter < 'a' || first_letter > 'z')
+			return -1;
+
+		if (start == first_letter)
 			continue;
-		start = *cur_funcname;
+		start = first_letter;
 		letter_offsets[(start & 31) - 1] = i;
 	}
 	return 0;
@@ -137,14 +148,15 @@ int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key)
 	u_short *p_keys, val;

 	if (letter_offsets[0] == -1)
-		help_init();
+		if (help_init())
+			return -1;
 	if (type == KT_LATIN) {
 		if (ch == SPACE) {
 			spk_special_handler = NULL;
 			synth_printf("%s\n", spk_msg_get(MSG_LEAVING_HELP));
 			return 1;
 		}
-		ch |= 32; /* lower case */
+		ch = tolower(ch);
 		if (ch < 'a' || ch > 'z')
 			return -1;
 		if (letter_offsets[ch - 'a'] == -1) {
--
2.43.0

Hello,

Thanks for checking this.

Samuel

Pavel Zhigulin, le ven. 26 sept. 2025 20:58:44 +0000, a ecrit:
> help_init() builds letter_offsets[] by using the first byte of each
> function name as an index via `(start & 31) - 1`. If function_names are
> overridden from sysfs (root) with a name starting outside [a–z], the
> index underflows or exceeds the array, leading to OOB write.
> 
> Function names can be overridden with the following commands as root:
> 
>     modprobe speakup_soft
>     echo "0 _bad" > /sys/accessibility/speakup/i18n/function_names
>     # then press Insert+2 on /dev/tty
> 
> This fix checks the first letter in help_init(), and if it is not in the
> [a–z] range the function returns an error to the caller. Eventually this
> error is propagated to drivers/accessibility/speakup/main.c:2217, which
> causes a bleep sound.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: a4efe6fd5dea ("staging: speakup: (coding style) Add spaces around operands (checkpatch checks)")

This reference is obviously wrong.

> Signed-off-by: Pavel Zhigulin <Pavel.Zhigulin@kaspersky.com>
> ---
>  drivers/accessibility/speakup/keyhelp.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/accessibility/speakup/keyhelp.c b/drivers/accessibility/speakup/keyhelp.c
> index 822ceac83068..df60a8b15a2f 100644
> --- a/drivers/accessibility/speakup/keyhelp.c
> +++ b/drivers/accessibility/speakup/keyhelp.c
> @@ -8,6 +8,7 @@
>   */
> 
>  #include <linux/keyboard.h>
> +#include <linux/ctype.h>
>  #include "spk_priv.h"
>  #include "speakup.h"
> 
> @@ -120,10 +121,20 @@ static int help_init(void)
>  	state_tbl = spk_our_keys[0] + SHIFT_TBL_SIZE + 2;
>  	for (i = 0; i < num_funcs; i++) {
>  		char *cur_funcname = spk_msg_get(MSG_FUNCNAMES_START + i);
> +		char first_letter;
> 
> -		if (start == *cur_funcname)
> +		if (!cur_funcname)

The function names array is not supposed to have null entries: they have
non-null defaults and cannot be cleared, at best defaulted back to the
default value.

> +			return -1;
> +
> +		first_letter = tolower(*cur_funcname);
> +
> +		/* Accept only 'a'..'z' to index letter_offsets[] safely */
> +		if (first_letter < 'a' || first_letter > 'z')
> +			return -1;

We don't want to make the help completely break just on odd function
name.

Better just continue the loop, the user will find the function in the
cur_item order anyway.

> +
> +		if (start == first_letter)
>  			continue;
> -		start = *cur_funcname;
> +		start = first_letter;
>  		letter_offsets[(start & 31) - 1] = i;
>  	}
>  	return 0;
> @@ -137,14 +148,15 @@ int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key)
>  	u_short *p_keys, val;
> 
>  	if (letter_offsets[0] == -1)
> -		help_init();
> +		if (help_init())
> +			return -1;

And then this is unnecessary. Actually help_init should just return
void.

>  	if (type == KT_LATIN) {
>  		if (ch == SPACE) {
>  			spk_special_handler = NULL;
>  			synth_printf("%s\n", spk_msg_get(MSG_LEAVING_HELP));
>  			return 1;
>  		}
> -		ch |= 32; /* lower case */
> +		ch = tolower(ch);
>  		if (ch < 'a' || ch > 'z')
>  			return -1;
>  		if (letter_offsets[ch - 'a'] == -1) {
> --
> 2.43.0
>