* [PATCH] accessibility: speakup: refactor deprecated strncpy @ 2023-08-24 21:44 Justin Stitt 2023-08-24 21:58 ` Samuel Thibault 2023-08-25 21:49 ` Kees Cook 0 siblings, 2 replies; 6+ messages in thread From: Justin Stitt @ 2023-08-24 21:44 UTC (permalink / raw) To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault Cc: speakup, linux-kernel, linux-hardening, Justin Stitt `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: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- 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 <justinstitt@google.com> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] accessibility: speakup: refactor deprecated strncpy 2023-08-24 21:44 [PATCH] accessibility: speakup: refactor deprecated strncpy Justin Stitt @ 2023-08-24 21:58 ` Samuel Thibault 2023-08-25 21:49 ` Kees Cook 1 sibling, 0 replies; 6+ messages in thread From: Samuel Thibault @ 2023-08-24 21:58 UTC (permalink / raw) To: Justin Stitt Cc: William Hubbs, Chris Brannon, Kirk Reiser, speakup, linux-kernel, linux-hardening 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: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > 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 <justinstitt@google.com> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] accessibility: speakup: refactor deprecated strncpy 2023-08-24 21:44 [PATCH] accessibility: speakup: refactor deprecated strncpy Justin Stitt 2023-08-24 21:58 ` Samuel Thibault @ 2023-08-25 21:49 ` Kees Cook 2023-08-25 22:41 ` Justin Stitt 1 sibling, 1 reply; 6+ messages in thread From: Kees Cook @ 2023-08-25 21:49 UTC (permalink / raw) To: Justin Stitt Cc: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault, speakup, linux-kernel, linux-hardening 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: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > 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 <keescook@chromium.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] accessibility: speakup: refactor deprecated strncpy 2023-08-25 21:49 ` Kees Cook @ 2023-08-25 22:41 ` Justin Stitt 2023-09-16 23:08 ` Samuel Thibault 0 siblings, 1 reply; 6+ messages in thread From: Justin Stitt @ 2023-08-25 22:41 UTC (permalink / raw) To: Kees Cook Cc: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault, speakup, linux-kernel, linux-hardening 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 <keescook@chromium.org> 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: linux-hardening@vger.kernel.org > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > 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 <keescook@chromium.org> > > 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 <justinstitt@google.com> 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 <justinstitt@google.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] accessibility: speakup: refactor deprecated strncpy 2023-08-25 22:41 ` Justin Stitt @ 2023-09-16 23:08 ` Samuel Thibault 2023-09-18 5:31 ` Justin Stitt 0 siblings, 1 reply; 6+ messages in thread From: Samuel Thibault @ 2023-09-16 23:08 UTC (permalink / raw) To: Justin Stitt Cc: Kees Cook, William Hubbs, Chris Brannon, Kirk Reiser, speakup, linux-kernel, linux-hardening 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 <justinstitt@google.com> > 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 <justinstitt@google.com> Tested-by: Samuel Thibault <samuel.thibault@ens-lyon.org> 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 > -- Samuel --- Pour une évaluation indépendante, transparente et rigoureuse ! Je soutiens la Commission d'Évaluation de l'Inria. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] accessibility: speakup: refactor deprecated strncpy 2023-09-16 23:08 ` Samuel Thibault @ 2023-09-18 5:31 ` Justin Stitt 0 siblings, 0 replies; 6+ messages in thread From: Justin Stitt @ 2023-09-18 5:31 UTC (permalink / raw) To: Samuel Thibault, Justin Stitt, Kees Cook, William Hubbs, Chris Brannon, Kirk Reiser, speakup, linux-kernel, linux-hardening On Sat, Sep 16, 2023 at 4:08 PM Samuel Thibault <samuel.thibault@ens-lyon.org> 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 <justinstitt@google.com> > > 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 <justinstitt@google.com> > > Tested-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > but please submit it properly :) It was completely mangled in the mail. Got it, here's a v2: https://lore.kernel.org/all/20230918-strncpy-drivers-accessibility-speakup-kobjects-c-v2-1-d5b1976c5dbf@google.com/ > > > --- > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-18 5:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-24 21:44 [PATCH] accessibility: speakup: refactor deprecated strncpy Justin Stitt 2023-08-24 21:58 ` Samuel Thibault 2023-08-25 21:49 ` Kees Cook 2023-08-25 22:41 ` Justin Stitt 2023-09-16 23:08 ` Samuel Thibault 2023-09-18 5:31 ` Justin Stitt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox