[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 27 Feb 2019 15:40:49 +1100
From: "Tobin C. Harding" <me@...in.cc>
To: Kees Cook <keescook@...omium.org>
Cc: "Tobin C. Harding" <tobin@...nel.org>, Jann Horn <jannh@...gle.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Randy Dunlap <rdunlap@...radead.org>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Stephen Rothwell <sfr@...b.auug.org.au>,
Andy Lutomirski <luto@...capital.net>,
Daniel Micay <danielmicay@...il.com>, Arnd Bergmann <arnd@...db.de>,
Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
"Gustavo A. R. Silva" <gustavo@...eddedor.com>,
Shuah Khan <shuah@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] lib/string: Add strscpy_pad() function
On Mon, Feb 25, 2019 at 01:38:44PM -0800, Kees Cook wrote:
> On Sun, Feb 24, 2019 at 8:16 PM Tobin C. Harding <tobin@...nel.org> wrote:
> >
> > We have a function to copy strings safely and we have a function to copy
> > strings and zero the tail of the destination (if source string is
> > shorter than destination buffer) but we do not have a function to do
> > both at once. This means developers must write this themselves if they
> > desire this functionality. This is a chore, and also leaves us open to
> > off by one errors unnecessarily.
> >
> > Add a function that calls strscpy() then memset()s the tail to zero if
> > the source string is shorter than the destination buffer.
> >
> > Add test module for the new code.
> >
> > Signed-off-by: Tobin C. Harding <tobin@...nel.org>
> > [...]
> > +ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> > +{
> > + ssize_t written;
> > +
> > + written = strscpy(dest, src, count);
> > + if (written < 0 || written == count - 1)
> > + return written;
>
> *thread merge* Yeah, good point. written will be -E2BIG for both count
> = 0 and count = 1.
>
> > +
> > + memset(dest + written + 1, 0, count - written - 1);
> > +
> > + return written;
> > +}
> > +EXPORT_SYMBOL(strscpy_pad);
> > +
> > #ifndef __HAVE_ARCH_STRCAT
> > /**
> > * strcat - Append one %NUL-terminated string to another
> > diff --git a/lib/test_strscpy.c b/lib/test_strscpy.c
> > new file mode 100644
> > index 000000000000..5ec6a196f4e2
> > --- /dev/null
> > +++ b/lib/test_strscpy.c
> > @@ -0,0 +1,175 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/printk.h>
> > +#include <linux/string.h>
> > +
> > +/*
> > + * Kernel module for testing 'strscpy' family of functions.
> > + */
> > +
> > +static unsigned total_tests __initdata;
> > +static unsigned failed_tests __initdata;
> > +
> > +static void __init do_test(int count, char *src, int expected,
> > + int chars, int terminator, int pad)
> > +{
> > + char buf[6];
>
> I would make this "6" a define, since you use it in more than one
> place. Actually... no, never mind. The other places can use
> "sizeof(buf)" instead of "6". Noted below...
>
> But I'd add an explicit check for "expected + 1 < sizeof(buf)" just
> for future test-addition sanity.
>
> > + int written;
> > + int poison;
> > + int index;
> > + int i;
> > + const char POISON = 'z';
> > +
> > + total_tests++;
> > + memset(buf, POISON, sizeof(buf));
> > +
> > + /* Verify the return value */
> > +
>
> Needless blank line.
>
> > + written = strscpy_pad(buf, src, count);
> > + if ((written) != (expected)) {
> > + pr_err("%d != %d (written, expected)\n", written, expected);
> > + goto fail;
> > + }
> > +
> > + /* Verify the state of the buffer */
> > +
>
> Same.
>
> > + if (count && written == -E2BIG) {
> > + if (strncmp(buf, src, count - 1) != 0) {
> > + pr_err("buffer state invalid for -E2BIG\n");
> > + goto fail;
> > + }
> > + if (buf[count - 1] != '\0') {
> > + pr_err("too big string is not null terminated correctly\n");
> > + goto fail;
> > + }
> > + }
> > +
> > + /* Verify the copied content */
> > + for (i = 0; i < chars; i++) {
> > + if (buf[i] != src[i]) {
> > + pr_err("buf[i]==%c != src[i]==%c\n", buf[i], src[i]);
> > + goto fail;
> > + }
> > + }
> > +
> > + /* Verify the null terminator */
> > + if (terminator) {
> > + if (buf[count - 1] != '\0') {
> > + pr_err("string is not null terminated correctly\n");
> > + goto fail;
> > + }
> > + }
> > +
> > + /* Verify the padding */
> > + for (i = 0; i < pad; i++) {
> > + index = chars + terminator + i;
> > + if (buf[index] != '\0') {
> > + pr_err("padding missing at index: %d\n", i);
> > + goto fail;
> > + }
> > + }
> > +
> > + /* Verify the rest is left untouched */
> > + poison = 6 - chars - terminator - pad;
>
> instead of "6", use "sizeof(buf)".
>
> > + for (i = 0; i < poison; i++) {
> > + index = 6 - 1 - i; /* Check from the end back */
>
> Same.
>
> > + if (buf[index] != POISON) {
> > + pr_err("poison value missing at index: %d\n", i);
> > + goto fail;
> > + }
> > + }
> > +
> > + return;
> > +fail:
> > + pr_info("%s(%d, '%s', %d, %d, %d, %d)\n", __func__,
> > + count, src, expected, chars, terminator, pad);
> > + failed_tests++;
> > +}
> > +
> > +static void __init test_fully(void)
> > +{
> > + /* do_test(count, src, expected, chars, terminator, pad) */
> > +
> > + do_test(0, "a", -E2BIG, 0, 0, 0);
> > + do_test(0, "", -E2BIG, 0, 0, 0);
> > +
> > + do_test(1, "a", -E2BIG, 0, 1, 0);
> > + do_test(1, "", 0, 0, 1, 0);
> > +
> > + do_test(2, "ab", -E2BIG, 1, 1, 0);
> > + do_test(2, "a", 1, 1, 1, 0);
> > + do_test(2, "", 0, 0, 1, 1);
> > +
> > + do_test(3, "abc", -E2BIG, 2, 1, 0);
> > + do_test(3, "ab", 2, 2, 1, 0);
> > + do_test(3, "a", 1, 1, 1, 1);
> > + do_test(3, "", 0, 0, 1, 2);
> > +
> > + do_test(4, "abcd", -E2BIG, 3, 1, 0);
> > + do_test(4, "abc", 3, 3, 1, 0);
> > + do_test(4, "ab", 2, 2, 1, 1);
> > + do_test(4, "a", 1, 1, 1, 2);
> > + do_test(4, "", 0, 0, 1, 3);
> > +}
> > +
> > +static void __init test_basic(void)
> > +{
> > + char buf[6];
> > + int written;
> > +
> > + memset(buf, 'a', sizeof(buf));
> > +
> > + total_tests++;
> > + written = strscpy_pad(buf, "bb", 4);
> > + if (written != 2)
> > + failed_tests++;
> > +
> > + /* Correctly copied */
> > + total_tests++;
> > + if (buf[0] != 'b' || buf[1] != 'b')
> > + failed_tests++;
> > +
> > + /* Correctly padded */
> > + total_tests++;
> > + if (buf[2] != '\0' || buf[3] != '\0')
> > + failed_tests++;
> > +
> > + /* Only touched what it was supposed to */
> > + total_tests++;
> > + if (buf[4] != 'a' || buf[5] != 'a')
> > + failed_tests++;
> > +}
>
> I don't think you need to keep "test_basic". Anything it tests can be
> rewritten in terms of do_test().
>
> > +
> > +static int __init test_strscpy_init(void)
> > +{
> > + pr_info("loaded.\n");
> > +
> > + test_basic();
> > + if (failed_tests)
> > + goto out;
> > +
> > + test_fully();
> > +
> > +out:
> > + if (failed_tests == 0)
> > + pr_info("all %u tests passed\n", total_tests);
> > + else
> > + pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
> > +
> > + return failed_tests ? -EINVAL : 0;
> > +}
> > +module_init(test_strscpy_init);
> > +
> > +static void __exit test_strscpy_exit(void)
> > +{
> > + pr_info("unloaded.\n");
> > +}
> > +module_exit(test_strscpy_exit);
> > +
> > +MODULE_AUTHOR("Tobin C. Harding <tobin@...nel.org>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.20.1
> >
>
> Otherwise, yes, looks good!
Cool, thanks. Will fix up as suggested and re-spin.
Tobin
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
