accessibility: speakup: refactor deprecated strncpy

Commit Message

Justin Stitt Aug. 24, 2023, 9:44 p.m. UTC

`strncpy` is deprecated for use on NUL-terminated destination strings [1].

Use `strscpy` as it guarantees NUL-termination of its destination buffer [2]
which allows for simpler and less ambiguous code.

Also, change `strlen(buf)` to `strlen(ptr)` to be consistent with
further usage within the scope of the function. Note that these are
equivalent:
|419 	const char *ptr = buf;

Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: [e-mail]
Signed-off-by: Justin Stitt <[e-mail]>
---
Note: build-tested only.
---
 drivers/accessibility/speakup/kobjects.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


---
base-commit: f9604036a3fb6149badf346994b46b03f9292db7
change-id: 20230824-strncpy-drivers-accessibility-speakup-kobjects-c-4009e7df0936

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Samuel Thibault Aug. 24, 2023, 9:58 p.m. UTC | #1

Justin Stitt, le jeu. 24 août 2023 21:44:29 +0000, a ecrit:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> Use `strscpy` as it guarantees NUL-termination of its destination buffer [2]
> which allows for simpler and less ambiguous code.
> 
> Also, change `strlen(buf)` to `strlen(ptr)` to be consistent with
> further usage within the scope of the function. Note that these are
> equivalent:
> |419 	const char *ptr = buf;
> 
> Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>

Reviewed-by: Samuel Thibault <[e-mail]>

> ---
> Note: build-tested only.
> ---
>  drivers/accessibility/speakup/kobjects.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
> index a7522d409802..8aa416c5f3fc 100644
> --- a/drivers/accessibility/speakup/kobjects.c
> +++ b/drivers/accessibility/speakup/kobjects.c
> @@ -422,12 +422,11 @@ static ssize_t synth_direct_store(struct kobject *kobj,
>  	if (!synth)
>  		return -EPERM;
>  
> -	len = strlen(buf);
> +	len = strlen(ptr);
>  	spin_lock_irqsave(&speakup_info.spinlock, flags);
>  	while (len > 0) {
>  		bytes = min_t(size_t, len, 250);
> -		strncpy(tmp, ptr, bytes);
> -		tmp[bytes] = '\0';
> +		strscpy(tmp, ptr, bytes);
>  		string_unescape_any_inplace(tmp);
>  		synth_printf("%s", tmp);
>  		ptr += bytes;
> 
> ---
> base-commit: f9604036a3fb6149badf346994b46b03f9292db7
> change-id: 20230824-strncpy-drivers-accessibility-speakup-kobjects-c-4009e7df0936
> 
> Best regards,
> --
> Justin Stitt <[e-mail]>
>

Kees Cook Aug. 25, 2023, 9:49 p.m. UTC | #2

On Thu, Aug 24, 2023 at 09:44:29PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> Use `strscpy` as it guarantees NUL-termination of its destination buffer [2]
> which allows for simpler and less ambiguous code.
> 
> Also, change `strlen(buf)` to `strlen(ptr)` to be consistent with
> further usage within the scope of the function. Note that these are
> equivalent:
> |419 	const char *ptr = buf;
> 
> Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>
> ---
> Note: build-tested only.
> ---
>  drivers/accessibility/speakup/kobjects.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
> index a7522d409802..8aa416c5f3fc 100644
> --- a/drivers/accessibility/speakup/kobjects.c
> +++ b/drivers/accessibility/speakup/kobjects.c
> @@ -422,12 +422,11 @@ static ssize_t synth_direct_store(struct kobject *kobj,
>  	if (!synth)
>  		return -EPERM;
>  
> -	len = strlen(buf);
> +	len = strlen(ptr);
>  	spin_lock_irqsave(&speakup_info.spinlock, flags);
>  	while (len > 0) {
>  		bytes = min_t(size_t, len, 250);
> -		strncpy(tmp, ptr, bytes);
> -		tmp[bytes] = '\0';
> +		strscpy(tmp, ptr, bytes);
>  		string_unescape_any_inplace(tmp);
>  		synth_printf("%s", tmp);
>  		ptr += bytes;

Technically, yes, this is fine...

Reviewed-by: Kees Cook <[e-mail]>

But wow do you find the most amazing code. :)

This thing is taking a buffer and chopping it up into at-most 250 byte
chunks (smaller than buf, I might add), and then sending it to
synth_printf() ... which uses a 160 byte buffer and silently
truncates...  and uses "%s" which is just a string copy...
why doesn't this just use synth_write() directly on an unescaped
buf??

I think this entire function should just be:

static ssize_t synth_direct_store(struct kobject *kobj,
                                  struct kobj_attribute *attr,
                                  const char *buf, size_t count)
{
        char *unescaped;
	char *p;

        if (!synth)
                return -EPERM;

	unescaped = kstrdup(buf, GFP_KERNEL);
	if (!unescaped)
		return -ENOMEM;

	string_unescape_any_inplace(unescaped);

        spin_lock_irqsave(&speakup_info.spinlock, flags);
	synth_write(unescaped, strlen(unescaped));
        spin_unlock_irqrestore(&speakup_info.spinlock, flags);

	kfree(unescaped);

        return count;
}

(Though honestly, why does this need unescaping anyway?)

-Kees

Justin Stitt Aug. 25, 2023, 10:41 p.m. UTC | #3

Thanks for the review Kees and Samuel. Hoping to get this picked-up soon :)

FWIW, I've quickly copy/pasted Kees' suggested refactor of
synth_direct_store and rebased against v6.5-rc7 if anyone has the
means by which to test it.

TEST PATCH BELOW

On Fri, Aug 25, 2023 at 2:49 PM Kees Cook <[e-mail]> wrote:
>
> On Thu, Aug 24, 2023 at 09:44:29PM +0000, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > Use `strscpy` as it guarantees NUL-termination of its destination buffer [2]
> > which allows for simpler and less ambiguous code.
> >
> > Also, change `strlen(buf)` to `strlen(ptr)` to be consistent with
> > further usage within the scope of the function. Note that these are
> > equivalent:
> > |419  const char *ptr = buf;
> >
> > Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: [e-mail]
> > Signed-off-by: Justin Stitt <[e-mail]>
> > ---
> > Note: build-tested only.
> > ---
> >  drivers/accessibility/speakup/kobjects.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
> > index a7522d409802..8aa416c5f3fc 100644
> > --- a/drivers/accessibility/speakup/kobjects.c
> > +++ b/drivers/accessibility/speakup/kobjects.c
> > @@ -422,12 +422,11 @@ static ssize_t synth_direct_store(struct kobject *kobj,
> >       if (!synth)
> >               return -EPERM;
> >
> > -     len = strlen(buf);
> > +     len = strlen(ptr);
> >       spin_lock_irqsave(&speakup_info.spinlock, flags);
> >       while (len > 0) {
> >               bytes = min_t(size_t, len, 250);
> > -             strncpy(tmp, ptr, bytes);
> > -             tmp[bytes] = '\0';
> > +             strscpy(tmp, ptr, bytes);
> >               string_unescape_any_inplace(tmp);
> >               synth_printf("%s", tmp);
> >               ptr += bytes;
>
> Technically, yes, this is fine...
>
> Reviewed-by: Kees Cook <[e-mail]>
>
> But wow do you find the most amazing code. :)
>
> This thing is taking a buffer and chopping it up into at-most 250 byte
> chunks (smaller than buf, I might add), and then sending it to
> synth_printf() ... which uses a 160 byte buffer and silently
> truncates...  and uses "%s" which is just a string copy...
> why doesn't this just use synth_write() directly on an unescaped
> buf??
>
> I think this entire function should just be:
>
> static ssize_t synth_direct_store(struct kobject *kobj,
>                                   struct kobj_attribute *attr,
>                                   const char *buf, size_t count)
> {
>         char *unescaped;
>         char *p;
>
>         if (!synth)
>                 return -EPERM;
>
>         unescaped = kstrdup(buf, GFP_KERNEL);
>         if (!unescaped)
>                 return -ENOMEM;
>
>         string_unescape_any_inplace(unescaped);
>
>         spin_lock_irqsave(&speakup_info.spinlock, flags);
>         synth_write(unescaped, strlen(unescaped));
>         spin_unlock_irqrestore(&speakup_info.spinlock, flags);
>
>         kfree(unescaped);
>
>         return count;
> }
>
> (Though honestly, why does this need unescaping anyway?)
>
> -Kees
>
> --
> Kees Cook
---
From e7216bca30673a162660c51f8bad3b463d283041 Mon Sep 17 00:00:00 2001
From: Justin Stitt <[e-mail]>
Date: Fri, 25 Aug 2023 22:32:03 +0000
Subject: [PATCH NEEDS TEST] synth_direct_store refactor to use synth_write

I've just copy/pasted Kees' suggestion here [1] and rebased it against
6.5-rc7.

This patch needs testing as it refactors behavior in synth_direct_store.

[1]: https://lore.kernel.org/all/202308251439.36BC33ADB2@keescook/

Signed-off-by: Justin Stitt <[e-mail]>
---
 drivers/accessibility/speakup/kobjects.c | 25 +++++++++++-------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/accessibility/speakup/kobjects.c
b/drivers/accessibility/speakup/kobjects.c
index a7522d409802..0dfdb6608e02 100644
--- a/drivers/accessibility/speakup/kobjects.c
+++ b/drivers/accessibility/speakup/kobjects.c
@@ -413,27 +413,24 @@ static ssize_t synth_direct_store(struct kobject *kobj,
    struct kobj_attribute *attr,
    const char *buf, size_t count)
 {
- u_char tmp[256];
- int len;
- int bytes;
- const char *ptr = buf;
+ char *unescaped;
  unsigned long flags;

  if (!synth)
  return -EPERM;

- len = strlen(buf);
+ unescaped = kstrdup(buf, GFP_KERNEL);
+ if (!unescaped)
+ return -ENOMEM;
+
+ string_unescape_any_inplace(unescaped);
+
  spin_lock_irqsave(&speakup_info.spinlock, flags);
- while (len > 0) {
- bytes = min_t(size_t, len, 250);
- strncpy(tmp, ptr, bytes);
- tmp[bytes] = '\0';
- string_unescape_any_inplace(tmp);
- synth_printf("%s", tmp);
- ptr += bytes;
- len -= bytes;
- }
+ synth_write(unescaped, strlen(unescaped));
  spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+
+ kfree(unescaped);
+
  return count;
 }

--
2.42.0.rc1.204.g551eb34607-goog

Samuel Thibault Sept. 16, 2023, 11:08 p.m. UTC | #4

Hello,

Justin Stitt, le ven. 25 août 2023 15:41:03 -0700, a ecrit:
> Thanks for the review Kees and Samuel. Hoping to get this picked-up soon :)
> 
> FWIW, I've quickly copy/pasted Kees' suggested refactor of
> synth_direct_store and rebased against v6.5-rc7 if anyone has the
> means by which to test it.
> 
> TEST PATCH BELOW
> ---
> From e7216bca30673a162660c51f8bad3b463d283041 Mon Sep 17 00:00:00 2001
> From: Justin Stitt <[e-mail]>
> Date: Fri, 25 Aug 2023 22:32:03 +0000
> Subject: [PATCH NEEDS TEST] synth_direct_store refactor to use synth_write
> 
> I've just copy/pasted Kees' suggestion here [1] and rebased it against
> 6.5-rc7.
> 
> This patch needs testing as it refactors behavior in synth_direct_store.
> 
> [1]: https://lore.kernel.org/all/202308251439.36BC33ADB2@keescook/
> 
> Signed-off-by: Justin Stitt <[e-mail]>

Tested-by: Samuel Thibault <[e-mail]>

but please submit it properly :) It was completely mangled in the mail.

> ---
>  drivers/accessibility/speakup/kobjects.c | 25 +++++++++++-------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/accessibility/speakup/kobjects.c
> b/drivers/accessibility/speakup/kobjects.c
> index a7522d409802..0dfdb6608e02 100644
> --- a/drivers/accessibility/speakup/kobjects.c
> +++ b/drivers/accessibility/speakup/kobjects.c
> @@ -413,27 +413,24 @@ static ssize_t synth_direct_store(struct kobject *kobj,
>     struct kobj_attribute *attr,
>     const char *buf, size_t count)
>  {
> - u_char tmp[256];
> - int len;
> - int bytes;
> - const char *ptr = buf;
> + char *unescaped;
>   unsigned long flags;
> 
>   if (!synth)
>   return -EPERM;
> 
> - len = strlen(buf);
> + unescaped = kstrdup(buf, GFP_KERNEL);
> + if (!unescaped)
> + return -ENOMEM;
> +
> + string_unescape_any_inplace(unescaped);
> +
>   spin_lock_irqsave(&speakup_info.spinlock, flags);
> - while (len > 0) {
> - bytes = min_t(size_t, len, 250);
> - strncpy(tmp, ptr, bytes);
> - tmp[bytes] = '\0';
> - string_unescape_any_inplace(tmp);
> - synth_printf("%s", tmp);
> - ptr += bytes;
> - len -= bytes;
> - }
> + synth_write(unescaped, strlen(unescaped));
>   spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +
> + kfree(unescaped);
> +
>   return count;
>  }
> 
> --
> 2.42.0.rc1.204.g551eb34607-goog
>

Justin Stitt Sept. 18, 2023, 5:31 a.m. UTC | #5

On Sat, Sep 16, 2023 at 4:08 PM Samuel Thibault
<[e-mail]> wrote:
>
> Hello,
>
> Justin Stitt, le ven. 25 août 2023 15:41:03 -0700, a ecrit:
> > Thanks for the review Kees and Samuel. Hoping to get this picked-up soon :)
> >
> > FWIW, I've quickly copy/pasted Kees' suggested refactor of
> > synth_direct_store and rebased against v6.5-rc7 if anyone has the
> > means by which to test it.
> >
> > TEST PATCH BELOW
> > ---
> > From e7216bca30673a162660c51f8bad3b463d283041 Mon Sep 17 00:00:00 2001
> > From: Justin Stitt <[e-mail]>
> > Date: Fri, 25 Aug 2023 22:32:03 +0000
> > Subject: [PATCH NEEDS TEST] synth_direct_store refactor to use synth_write
> >
> > I've just copy/pasted Kees' suggestion here [1] and rebased it against
> > 6.5-rc7.
> >
> > This patch needs testing as it refactors behavior in synth_direct_store.
> >
> > [1]: https://lore.kernel.org/all/202308251439.36BC33ADB2@keescook/
> >
> > Signed-off-by: Justin Stitt <[e-mail]>
>
> Tested-by: Samuel Thibault <[e-mail]>
>
> but please submit it properly :) It was completely mangled in the mail.

Got it, here's a v2:
https://lore.kernel.org/all/20230918-st[e-mail]/

>
> > ---
> >  drivers/accessibility/speakup/kobjects.c | 25 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/accessibility/speakup/kobjects.c
> > b/drivers/accessibility/speakup/kobjects.c
> > index a7522d409802..0dfdb6608e02 100644
> > --- a/drivers/accessibility/speakup/kobjects.c
> > +++ b/drivers/accessibility/speakup/kobjects.c
> > @@ -413,27 +413,24 @@ static ssize_t synth_direct_store(struct kobject *kobj,
> >     struct kobj_attribute *attr,
> >     const char *buf, size_t count)
> >  {
> > - u_char tmp[256];
> > - int len;
> > - int bytes;
> > - const char *ptr = buf;
> > + char *unescaped;
> >   unsigned long flags;
> >
> >   if (!synth)
> >   return -EPERM;
> >
> > - len = strlen(buf);
> > + unescaped = kstrdup(buf, GFP_KERNEL);
> > + if (!unescaped)
> > + return -ENOMEM;
> > +
> > + string_unescape_any_inplace(unescaped);
> > +
> >   spin_lock_irqsave(&speakup_info.spinlock, flags);
> > - while (len > 0) {
> > - bytes = min_t(size_t, len, 250);
> > - strncpy(tmp, ptr, bytes);
> > - tmp[bytes] = '\0';
> > - string_unescape_any_inplace(tmp);
> > - synth_printf("%s", tmp);
> > - ptr += bytes;
> > - len -= bytes;
> > - }
> > + synth_write(unescaped, strlen(unescaped));
> >   spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> > +
> > + kfree(unescaped);
> > +
> >   return count;
> >  }
> >
> > --
> > 2.42.0.rc1.204.g551eb34607-goog
> >
>
> --
> Samuel
> ---
> Pour une évaluation indépendante, transparente et rigoureuse !
> Je soutiens la Commission d'Évaluation de l'Inria.

Patch

diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
index a7522d409802..8aa416c5f3fc 100644
--- a/drivers/accessibility/speakup/kobjects.c
+++ b/drivers/accessibility/speakup/kobjects.c
@@ -422,12 +422,11 @@  static ssize_t synth_direct_store(struct kobject *kobj,
 	if (!synth)
 		return -EPERM;
 
-	len = strlen(buf);
+	len = strlen(ptr);
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
 	while (len > 0) {
 		bytes = min_t(size_t, len, 250);
-		strncpy(tmp, ptr, bytes);
-		tmp[bytes] = '\0';
+		strscpy(tmp, ptr, bytes);
 		string_unescape_any_inplace(tmp);
 		synth_printf("%s", tmp);
 		ptr += bytes;