lkml.org 
[lkml]   [2019]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@kernel.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@kernel.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@kernel.org>");
    > > +MODULE_LICENSE("GPL");
    > > --
    > > 2.20.1
    > >
    >
    > Otherwise, yes, looks good!

    Cool, thanks. Will fix up as suggested and re-spin.

    Tobin

    \
     
     \ /
      Last update: 2019-02-27 05:41    [W:4.469 / U:0.516 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site