From: Kees Cook <keescook@chromium.org>
To: "J. Bruce Fields" <bfields@redhat.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/9] Eliminate unused ESCAPE_NULL, ESCAPE_SPACE flags
Date: Thu, 5 Sep 2019 15:11:08 -0700 [thread overview]
Message-ID: <201909051437.56B9B68@keescook> (raw)
In-Reply-To: <1567712673-1629-6-git-send-email-bfields@redhat.com>
On Thu, Sep 05, 2019 at 03:44:30PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@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 <bfields@redhat.com>
Acked-by: Kees Cook <keescook@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
next prev parent reply other threads:[~2019-09-05 22:11 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190905193604.GC31247@fieldses.org>
2019-09-05 19:44 ` [PATCH 1/9] rtl8192*: display ESSIDs using %pE J. Bruce Fields
2019-09-05 19:44 ` [PATCH 2/9] thunderbolt: show key using %*s not %*pE J. Bruce Fields
2019-09-05 21:17 ` Kees Cook
2019-09-09 19:34 ` Joe Perches
2019-09-05 19:44 ` [PATCH 3/9] staging: wlan-ng: use "%*pE" for serial number J. Bruce Fields
2019-09-05 21:30 ` Kees Cook
2019-09-05 19:44 ` [PATCH 4/9] Remove unused string_escape_*_any_np J. Bruce Fields
2019-09-05 21:32 ` Kees Cook
2019-09-05 19:44 ` [PATCH 5/9] Remove unused %*pE[achnops] formats J. Bruce Fields
2019-09-05 21:34 ` Kees Cook
2019-09-06 10:01 ` Andy Shevchenko
2019-09-06 10:03 ` Andy Shevchenko
2019-09-05 19:44 ` [PATCH 6/9] Eliminate unused ESCAPE_NULL, ESCAPE_SPACE flags J. Bruce Fields
2019-09-05 22:11 ` Kees Cook [this message]
2019-09-06 10:17 ` Andy Shevchenko
2019-09-05 19:44 ` [PATCH 7/9] Simplify string_escape_mem J. Bruce Fields
2019-09-05 22:29 ` Kees Cook
2019-09-05 19:44 ` [PATCH 8/9] minor kstrdup_quotable simplification J. Bruce Fields
2019-09-05 22:31 ` Kees Cook
2019-09-05 19:44 ` [PATCH 9/9] Remove string_escape_mem_ascii J. Bruce Fields
2019-09-05 22:34 ` Kees Cook
2019-09-06 10:20 ` Andy Shevchenko
2019-09-05 20:53 ` [PATCH 1/9] rtl8192*: display ESSIDs using %pE Kees Cook
2019-09-06 9:38 ` Andy Shevchenko
2019-09-06 15:53 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201909051437.56B9B68@keescook \
--to=keescook@chromium.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bfields@redhat.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.