[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAFhGd8r6A4VH5C=yF1WHKEPY86oh6PEzt6wuxPxsJAD0XKfrsQ@mail.gmail.com>
Date: Fri, 25 Aug 2023 15:41:03 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Kees Cook <keescook@...omium.org>
Cc: William Hubbs <w.d.hubbs@...il.com>,
Chris Brannon <chris@...-brannons.com>,
Kirk Reiser <kirk@...sers.ca>,
Samuel Thibault <samuel.thibault@...-lyon.org>,
speakup@...ux-speakup.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH] accessibility: speakup: refactor deprecated strncpy
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@...omium.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@...r.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@...gle.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@...omium.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@...gle.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@...gle.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
Powered by blists - more mailing lists