On Thu, Sep 05, 2019 at 03:44:30PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfie...@redhat.com>
>
> I can see how some finer-grained flags could be useful, but for now I'm
> trying to simplify, so let's just remove these unused ones.
I might change the Subject to "merge ESCAPE_{NULL,SPACE,SPECIAL}" or so.
> Note the trickiest part is actually the tests, and I still need to check
> them.
It looks correct to me, though I haven't run them myself yet. One
thing to add might be adding a NULL test with explicit calls to
string_escape_mem()?
>
> Signed-off-by: J. Bruce Fields <bfie...@redhat.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> fs/proc/array.c | 2 +-
> include/linux/string_helpers.h | 6 ++--
> lib/string_helpers.c | 54 +++----------------------------
> lib/test-string_helpers.c | 58 ++++------------------------------
> 4 files changed, 14 insertions(+), 106 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 46dcb6f0eccf..982c95d09176 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -111,7 +111,7 @@ void proc_task_name(struct seq_file *m, struct
> task_struct *p, bool escape)
> size = seq_get_buf(m, &buf);
> if (escape) {
> ret = string_escape_str(tcomm, buf, size,
> - ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
> + ESCAPE_SPECIAL, "\n\\");
> if (ret >= size)
> ret = -1;
> } else {
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 8a299a29b767..7bf0d137373d 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -41,15 +41,13 @@ static inline int string_unescape_any_inplace(char *buf)
> return string_unescape_any(buf, buf, 0);
> }
>
> -#define ESCAPE_SPACE 0x01
> #define ESCAPE_SPECIAL 0x02
> -#define ESCAPE_NULL 0x04
> #define ESCAPE_OCTAL 0x08
> -#define ESCAPE_ANY \
> - (ESCAPE_SPACE | ESCAPE_OCTAL | ESCAPE_SPECIAL | ESCAPE_NULL)
> +#define ESCAPE_ANY (ESCAPE_OCTAL | ESCAPE_SPECIAL)
> #define ESCAPE_NP 0x10
> #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP)
> #define ESCAPE_HEX 0x20
> +#define ESCAPE_FLAG_MASK 0x3a
>
> int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
> unsigned int flags, const char *only);
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 963050c0283e..ac72159d3980 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -310,7 +310,7 @@ static bool escape_passthrough(unsigned char c, char
> **dst, char *end)
> return true;
> }
>
> -static bool escape_space(unsigned char c, char **dst, char *end)
> +static bool escape_special(unsigned char c, char **dst, char *end)
> {
> char *out = *dst;
> unsigned char to;
> @@ -331,27 +331,6 @@ static bool escape_space(unsigned char c, char **dst,
> char *end)
> case '\f':
> to = 'f';
> break;
> - default:
> - return false;
> - }
> -
> - if (out < end)
> - *out = '\\';
> - ++out;
> - if (out < end)
> - *out = to;
> - ++out;
> -
> - *dst = out;
> - return true;
> -}
> -
> -static bool escape_special(unsigned char c, char **dst, char *end)
> -{
> - char *out = *dst;
> - unsigned char to;
> -
> - switch (c) {
> case '\\':
> to = '\\';
> break;
> @@ -361,6 +340,9 @@ static bool escape_special(unsigned char c, char **dst,
> char *end)
> case '\e':
> to = 'e';
> break;
> + case '\0':
> + to = '0';
> + break;
> default:
> return false;
> }
> @@ -376,24 +358,6 @@ static bool escape_special(unsigned char c, char **dst,
> char *end)
> return true;
> }
>
> -static bool escape_null(unsigned char c, char **dst, char *end)
> -{
> - char *out = *dst;
> -
> - if (c)
> - return false;
> -
> - if (out < end)
> - *out = '\\';
> - ++out;
> - if (out < end)
> - *out = '0';
> - ++out;
> -
> - *dst = out;
> - return true;
> -}
> -
> static bool escape_octal(unsigned char c, char **dst, char *end)
> {
> char *out = *dst;
> @@ -465,17 +429,15 @@ static bool escape_hex(unsigned char c, char **dst,
> char *end)
> * destination buffer will not be NULL-terminated, thus caller have to append
> * it if needs. The supported flags are::
> *
> - * %ESCAPE_SPACE: (special white space, not space itself)
> + * %ESCAPE_SPECIAL:
> * '\f' - form feed
> * '\n' - new line
> * '\r' - carriage return
> * '\t' - horizontal tab
> * '\v' - vertical tab
> - * %ESCAPE_SPECIAL:
> * '\\' - backslash
> * '\a' - alert (BEL)
> * '\e' - escape
> - * %ESCAPE_NULL:
> * '\0' - null
> * %ESCAPE_OCTAL:
> * '\NNN' - byte with octal value NNN (3 digits)
> @@ -519,15 +481,9 @@ int string_escape_mem(const char *src, size_t isz, char
> *dst, size_t osz,
> (is_dict && !strchr(only, c))) {
> /* do nothing */
> } else {
> - if (flags & ESCAPE_SPACE && escape_space(c, &p, end))
> - continue;
> -
> if (flags & ESCAPE_SPECIAL && escape_special(c, &p,
> end))
> continue;
>
> - if (flags & ESCAPE_NULL && escape_null(c, &p, end))
> - continue;
> -
> /* ESCAPE_OCTAL and ESCAPE_HEX always go last */
> if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end))
> continue;
> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
> index 25b5cbfb7615..0da3c195a327 100644
> --- a/lib/test-string_helpers.c
> +++ b/lib/test-string_helpers.c
> @@ -124,20 +124,6 @@ struct test_string_2 {
>
> #define TEST_STRING_2_DICT_0 NULL
> static const struct test_string_2 escape0[] __initconst = {{
> - .in = "\f\\ \n\r\t\v",
> - .s1 = {{
> - .out = "\\f\\ \\n\\r\\t\\v",
> - .flags = ESCAPE_SPACE,
> - },{
> - .out = "\\f\\134\\040\\n\\r\\t\\v",
> - .flags = ESCAPE_SPACE | ESCAPE_OCTAL,
> - },{
> - .out = "\\f\\x5c\\x20\\n\\r\\t\\v",
> - .flags = ESCAPE_SPACE | ESCAPE_HEX,
> - },{
> - /* terminator */
> - }},
> -},{
> .in = "\\h\\\"\a\e\\",
> .s1 = {{
> .out = "\\\\h\\\\\"\\a\\e\\\\",
> @@ -154,48 +140,26 @@ static const struct test_string_2 escape0[] __initconst
> = {{
> },{
> .in = "\eb \\C\007\"\x90\r]",
> .s1 = {{
> - .out = "\eb \\C\007\"\x90\\r]",
> - .flags = ESCAPE_SPACE,
> - },{
> - .out = "\\eb \\\\C\\a\"\x90\r]",
> - .flags = ESCAPE_SPECIAL,
> - },{
> .out = "\\eb \\\\C\\a\"\x90\\r]",
> - .flags = ESCAPE_SPACE | ESCAPE_SPECIAL,
> + .flags = ESCAPE_SPECIAL,
> },{
> .out = "\\033\\142\\040\\134\\103\\007\\042\\220\\015\\135",
> .flags = ESCAPE_OCTAL,
> - },{
> - .out = "\\033\\142\\040\\134\\103\\007\\042\\220\\r\\135",
> - .flags = ESCAPE_SPACE | ESCAPE_OCTAL,
> - },{
> - .out = "\\e\\142\\040\\\\\\103\\a\\042\\220\\015\\135",
> - .flags = ESCAPE_SPECIAL | ESCAPE_OCTAL,
> },{
> .out = "\\e\\142\\040\\\\\\103\\a\\042\\220\\r\\135",
> - .flags = ESCAPE_SPACE | ESCAPE_SPECIAL | ESCAPE_OCTAL,
> + .flags = ESCAPE_SPECIAL | ESCAPE_OCTAL,
> },{
> .out = "\eb \\C\007\"\x90\r]",
> .flags = ESCAPE_NP,
> - },{
> - .out = "\eb \\C\007\"\x90\\r]",
> - .flags = ESCAPE_SPACE | ESCAPE_NP,
> - },{
> - .out = "\\eb \\C\\a\"\x90\r]",
> - .flags = ESCAPE_SPECIAL | ESCAPE_NP,
> },{
> .out = "\\eb \\C\\a\"\x90\\r]",
> - .flags = ESCAPE_SPACE | ESCAPE_SPECIAL | ESCAPE_NP,
> + .flags = ESCAPE_SPECIAL | ESCAPE_NP,
> },{
> .out = "\\033b \\C\\007\"\\220\\015]",
> .flags = ESCAPE_OCTAL | ESCAPE_NP,
> - },{
> - .out = "\\033b \\C\\007\"\\220\\r]",
> - .flags = ESCAPE_SPACE | ESCAPE_OCTAL | ESCAPE_NP,
> },{
> .out = "\\eb \\C\\a\"\\220\\r]",
> - .flags = ESCAPE_SPECIAL | ESCAPE_SPACE | ESCAPE_OCTAL |
> - ESCAPE_NP,
> + .flags = ESCAPE_SPECIAL ESCAPE_OCTAL | ESCAPE_NP,
> },{
> .out = "\\x1bb \\C\\x07\"\\x90\\x0d]",
> .flags = ESCAPE_NP | ESCAPE_HEX,
> @@ -247,9 +211,6 @@ static __init const char *test_string_find_match(const
> struct test_string_2 *s2,
> if (!flags)
> return s2->in;
>
> - /* Test cases are NULL-aware */
> - flags &= ~ESCAPE_NULL;
> -
> /* ESCAPE_OCTAL has a higher priority */
> if (flags & ESCAPE_OCTAL)
> flags &= ~ESCAPE_HEX;
> @@ -290,13 +251,6 @@ static __init void test_string_escape(const char *name,
> const char *out;
> int len;
>
> - /* NULL injection */
> - if (flags & ESCAPE_NULL) {
> - in[p++] = '\0';
> - out_test[q_test++] = '\\';
> - out_test[q_test++] = '0';
> - }
> -
> /* Don't try strings that have no output */
> out = test_string_find_match(s2, flags);
> if (!out)
> @@ -401,11 +355,11 @@ static int __init test_string_helpers_init(void)
> get_random_int() % (UNESCAPE_ANY + 1), true);
>
> /* Without dictionary */
> - for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
> + for (i = 0; i < ESCAPE_FLAG_MASK + 1; i++)
> test_string_escape("escape 0", escape0, i,
> TEST_STRING_2_DICT_0);
>
> /* With dictionary */
> - for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
> + for (i = 0; i < ESCAPE_FLAG_MASK + 1; i++)
> test_string_escape("escape 1", escape1, i,
> TEST_STRING_2_DICT_1);
>
> /* Test string_get_size() */
> --
> 2.21.0
>
--
Kees Cook
