[PATCH 1/9] rtl8192*: display ESSIDs using %pE

[PATCH 1/9] rtl8192*: display ESSIDs using %pE

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Everywhere else in the kernel ESSIDs are printed using %pE, and I can't
see why there should be an exception here.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 drivers/staging/rtl8192e/rtllib.h              | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
index 2dd57e88276e..096254e422b3 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -2132,7 +2132,7 @@ static inline const char *escape_essid(const char *essid, u8 essid_len)
 		return escaped;
 	}
 
-	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
+	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);
 	return escaped;
 }
 
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
index d36963469015..3963a08b9eb2 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
@@ -2426,7 +2426,7 @@ static inline const char *escape_essid(const char *essid, u8 essid_len)
 		return escaped;
 	}
 
-	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
+	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);
 	return escaped;
 }
 
-- 
2.21.0

[PATCH 2/9] thunderbolt: show key using %*s not %*pE

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

%*pEp (without "h" or "o") is a no-op.  This string could contain
arbitrary (non-NULL) characters, so we do want escaping.  Use %*pE like
every other caller.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 drivers/thunderbolt/xdomain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 5118d46702d5..4e17a7c7bf0a 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -636,7 +636,7 @@ static ssize_t key_show(struct device *dev, struct device_attribute *attr,
 	 * It should be null terminated but anything else is pretty much
 	 * allowed.
 	 */
-	return sprintf(buf, "%*pEp\n", (int)strlen(svc->key), svc->key);
+	return sprintf(buf, "%*pE\n", (int)strlen(svc->key), svc->key);
 }
 static DEVICE_ATTR_RO(key);
 
-- 
2.21.0

Re: [PATCH 2/9] thunderbolt: show key using %*s not %*pE

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:26PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> %*pEp (without "h" or "o") is a no-op.  This string could contain
> arbitrary (non-NULL) characters, so we do want escaping.  Use %*pE like
> every other caller.

Agreed on all counts. pEp is actively resulting in NO escaping, which is
a bug here.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  drivers/thunderbolt/xdomain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
> index 5118d46702d5..4e17a7c7bf0a 100644
> --- a/drivers/thunderbolt/xdomain.c
> +++ b/drivers/thunderbolt/xdomain.c
> @@ -636,7 +636,7 @@ static ssize_t key_show(struct device *dev, struct device_attribute *attr,
>  	 * It should be null terminated but anything else is pretty much
>  	 * allowed.
>  	 */
> -	return sprintf(buf, "%*pEp\n", (int)strlen(svc->key), svc->key);
> +	return sprintf(buf, "%*pE\n", (int)strlen(svc->key), svc->key);
>  }
>  static DEVICE_ATTR_RO(key);
>  
> -- 
> 2.21.0
> 

-- 
Kees Cook

Re: [PATCH 2/9] thunderbolt: show key using %*s not %*pE

[Joe Perches]

On Thu, 2019-09-05 at 15:44 -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> %*pEp (without "h" or "o") is a no-op.  This string could contain
> arbitrary (non-NULL) characters, so we do want escaping.  Use %*pE like
> every other caller.
[]
> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
[]
> @@ -636,7 +636,7 @@ static ssize_t key_show(struct device *dev, struct device_attribute *attr,
>  	 * It should be null terminated but anything else is pretty much
>  	 * allowed.
>  	 */
> -	return sprintf(buf, "%*pEp\n", (int)strlen(svc->key), svc->key);
> +	return sprintf(buf, "%*pE\n", (int)strlen(svc->key), svc->key);

The code does not match the patch subject / commit title.

[PATCH 3/9] staging: wlan-ng: use "%*pE" for serial number

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Almost every user of "%*pE" in the kernel uses just bare "%*pE".  This
is the only user of "%pEhp".  I can't see why it's needed.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 drivers/staging/wlan-ng/prism2sta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wlan-ng/prism2sta.c b/drivers/staging/wlan-ng/prism2sta.c
index fb5441399131..8f25496188aa 100644
--- a/drivers/staging/wlan-ng/prism2sta.c
+++ b/drivers/staging/wlan-ng/prism2sta.c
@@ -846,7 +846,7 @@ static int prism2sta_getcardinfo(struct wlandevice *wlandev)
 	result = hfa384x_drvr_getconfig(hw, HFA384x_RID_NICSERIALNUMBER,
 					snum, HFA384x_RID_NICSERIALNUMBER_LEN);
 	if (!result) {
-		netdev_info(wlandev->netdev, "Prism2 card SN: %*pEhp\n",
+		netdev_info(wlandev->netdev, "Prism2 card SN: %*pE\n",
 			    HFA384x_RID_NICSERIALNUMBER_LEN, snum);
 	} else {
 		netdev_err(wlandev->netdev, "Failed to retrieve Prism2 Card SN\n");
-- 
2.21.0

Re: [PATCH 3/9] staging: wlan-ng: use "%*pE" for serial number

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:27PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Almost every user of "%*pE" in the kernel uses just bare "%*pE".  This
> is the only user of "%pEhp".  I can't see why it's needed.

Agreed, though to be clear, before, I think every byte in the string
is hex-escaped. After this patch, the space and specials will get
character-based escapes and everything else will switch to octal escapes.

i.e. a string of newline, capital-a, NUL will change from "\x0a\x41" to
"\n\101".

Given that this is only reported to dmesg, it is probably fine. Also,
it's staging and prism2 ... is anyone actually using this?

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  drivers/staging/wlan-ng/prism2sta.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wlan-ng/prism2sta.c b/drivers/staging/wlan-ng/prism2sta.c
> index fb5441399131..8f25496188aa 100644
> --- a/drivers/staging/wlan-ng/prism2sta.c
> +++ b/drivers/staging/wlan-ng/prism2sta.c
> @@ -846,7 +846,7 @@ static int prism2sta_getcardinfo(struct wlandevice *wlandev)
>  	result = hfa384x_drvr_getconfig(hw, HFA384x_RID_NICSERIALNUMBER,
>  					snum, HFA384x_RID_NICSERIALNUMBER_LEN);
>  	if (!result) {
> -		netdev_info(wlandev->netdev, "Prism2 card SN: %*pEhp\n",
> +		netdev_info(wlandev->netdev, "Prism2 card SN: %*pE\n",
>  			    HFA384x_RID_NICSERIALNUMBER_LEN, snum);
>  	} else {
>  		netdev_err(wlandev->netdev, "Failed to retrieve Prism2 Card SN\n");
> -- 
> 2.21.0
> 

-- 
Kees Cook

[PATCH 4/9] Remove unused string_escape_*_any_np

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

These aren't called anywhere.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 include/linux/string_helpers.h | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index c28955132234..8a299a29b767 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -56,25 +56,12 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 
 int string_escape_mem_ascii(const char *src, size_t isz, char *dst,
 					size_t osz);
-
-static inline int string_escape_mem_any_np(const char *src, size_t isz,
-		char *dst, size_t osz, const char *only)
-{
-	return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, only);
-}
-
 static inline int string_escape_str(const char *src, char *dst, size_t sz,
 		unsigned int flags, const char *only)
 {
 	return string_escape_mem(src, strlen(src), dst, sz, flags, only);
 }
 
-static inline int string_escape_str_any_np(const char *src, char *dst,
-		size_t sz, const char *only)
-{
-	return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, only);
-}
-
 char *kstrdup_quotable(const char *src, gfp_t gfp);
 char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp);
 char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
-- 
2.21.0

Re: [PATCH 4/9] Remove unused string_escape_*_any_np

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:28PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> These aren't called anywhere.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  include/linux/string_helpers.h | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index c28955132234..8a299a29b767 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -56,25 +56,12 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
>  
>  int string_escape_mem_ascii(const char *src, size_t isz, char *dst,
>  					size_t osz);
> -
> -static inline int string_escape_mem_any_np(const char *src, size_t isz,
> -		char *dst, size_t osz, const char *only)
> -{
> -	return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, only);
> -}
> -
>  static inline int string_escape_str(const char *src, char *dst, size_t sz,
>  		unsigned int flags, const char *only)
>  {
>  	return string_escape_mem(src, strlen(src), dst, sz, flags, only);
>  }
>  
> -static inline int string_escape_str_any_np(const char *src, char *dst,
> -		size_t sz, const char *only)
> -{
> -	return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, only);
> -}
> -
>  char *kstrdup_quotable(const char *src, gfp_t gfp);
>  char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp);
>  char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
> -- 
> 2.21.0
> 

-- 
Kees Cook

[PATCH 5/9] Remove unused %*pE[achnops] formats

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

The [achnops] are confusing, and in practice the only one anyone seems
to need is the bare %*pE.

I think some set of modifiers here might actually be useful, but the
ones we have are confusing and unused, so let's just toss these out and
then rethink what we might want to add back later.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 Documentation/core-api/printk-formats.rst | 23 ++---------
 lib/vsprintf.c                            | 50 ++---------------------
 2 files changed, 7 insertions(+), 66 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..4f9d20dfb342 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -180,35 +180,20 @@ Raw buffer as an escaped string
 
 ::
 
-	%*pE[achnops]
+	%*pE
 
 For printing raw buffer as an escaped string. For the following buffer::
 
 		1b 62 20 5c 43 07 22 90 0d 5d
 
-A few examples show how the conversion would be done (excluding surrounding
+An example shows how the conversion would be done (excluding surrounding
 quotes)::
 
 		%*pE		"\eb \C\a"\220\r]"
-		%*pEhp		"\x1bb \C\x07"\x90\x0d]"
-		%*pEa		"\e\142\040\\\103\a\042\220\r\135"
 
-The conversion rules are applied according to an optional combination
-of flags (see :c:func:`string_escape_mem` kernel documentation for the
-details):
+See :c:func:`string_escape_mem` kernel documentation for the details.
 
-	- a - ESCAPE_ANY
-	- c - ESCAPE_SPECIAL
-	- h - ESCAPE_HEX
-	- n - ESCAPE_NULL
-	- o - ESCAPE_OCTAL
-	- p - ESCAPE_NP
-	- s - ESCAPE_SPACE
-
-By default ESCAPE_ANY_NP is used.
-
-ESCAPE_ANY_NP is the sane choice for many cases, in particularly for
-printing SSIDs.
+This is used, for example, for printing SSIDs.
 
 If field width is omitted then 1 byte only will be escaped.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..5522d2a052e1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1537,9 +1537,6 @@ static noinline_for_stack
 char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		     const char *fmt)
 {
-	bool found = true;
-	int count = 1;
-	unsigned int flags = 0;
 	int len;
 
 	if (spec.field_width == 0)
@@ -1548,38 +1545,6 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	if (check_pointer(&buf, end, addr, spec))
 		return buf;
 
-	do {
-		switch (fmt[count++]) {
-		case 'a':
-			flags |= ESCAPE_ANY;
-			break;
-		case 'c':
-			flags |= ESCAPE_SPECIAL;
-			break;
-		case 'h':
-			flags |= ESCAPE_HEX;
-			break;
-		case 'n':
-			flags |= ESCAPE_NULL;
-			break;
-		case 'o':
-			flags |= ESCAPE_OCTAL;
-			break;
-		case 'p':
-			flags |= ESCAPE_NP;
-			break;
-		case 's':
-			flags |= ESCAPE_SPACE;
-			break;
-		default:
-			found = false;
-			break;
-		}
-	} while (found);
-
-	if (!flags)
-		flags = ESCAPE_ANY_NP;
-
 	len = spec.field_width < 0 ? 1 : spec.field_width;
 
 	/*
@@ -1587,7 +1552,8 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	 * the given buffer, and returns the total size of the output
 	 * had the buffer been big enough.
 	 */
-	buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL);
+	buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0,
+				 ESCAPE_ANY_NP, NULL);
 
 	return buf;
 }
@@ -2038,17 +2004,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
  * - '[Ii][4S][hnbl]' IPv4 addresses in host, network, big or little endian order
  * - 'I[6S]c' for IPv6 addresses printed as specified by
  *       http://tools.ietf.org/html/rfc5952
- * - 'E[achnops]' For an escaped buffer, where rules are defined by combination
- *                of the following flags (see string_escape_mem() for the
- *                details):
- *                  a - ESCAPE_ANY
- *                  c - ESCAPE_SPECIAL
- *                  h - ESCAPE_HEX
- *                  n - ESCAPE_NULL
- *                  o - ESCAPE_OCTAL
- *                  p - ESCAPE_NP
- *                  s - ESCAPE_SPACE
- *                By default ESCAPE_ANY_NP is used.
+ * - 'E[achnops]' For an escaped buffer (see string_escape_mem()
  * - 'U' For a 16 byte UUID/GUID, it prints the UUID/GUID in the form
  *       "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
  *       Options for %pU are:
-- 
2.21.0

Re: [PATCH 5/9] Remove unused %*pE[achnops] formats

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:29PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> The [achnops] are confusing, and in practice the only one anyone seems
> to need is the bare %*pE.
> 
> I think some set of modifiers here might actually be useful, but the
> ones we have are confusing and unused, so let's just toss these out and
> then rethink what we might want to add back later.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Acked-by: Kees Cook <keescook@chromium.org>

Typo below...

> ---
>  Documentation/core-api/printk-formats.rst | 23 ++---------
>  lib/vsprintf.c                            | 50 ++---------------------
>  2 files changed, 7 insertions(+), 66 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcb..4f9d20dfb342 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -180,35 +180,20 @@ Raw buffer as an escaped string
>  
>  ::
>  
> -	%*pE[achnops]
> +	%*pE
>  
>  For printing raw buffer as an escaped string. For the following buffer::
>  
>  		1b 62 20 5c 43 07 22 90 0d 5d
>  
> -A few examples show how the conversion would be done (excluding surrounding
> +An example shows how the conversion would be done (excluding surrounding
>  quotes)::
>  
>  		%*pE		"\eb \C\a"\220\r]"
> -		%*pEhp		"\x1bb \C\x07"\x90\x0d]"
> -		%*pEa		"\e\142\040\\\103\a\042\220\r\135"
>  
> -The conversion rules are applied according to an optional combination
> -of flags (see :c:func:`string_escape_mem` kernel documentation for the
> -details):
> +See :c:func:`string_escape_mem` kernel documentation for the details.
>  
> -	- a - ESCAPE_ANY
> -	- c - ESCAPE_SPECIAL
> -	- h - ESCAPE_HEX
> -	- n - ESCAPE_NULL
> -	- o - ESCAPE_OCTAL
> -	- p - ESCAPE_NP
> -	- s - ESCAPE_SPACE
> -
> -By default ESCAPE_ANY_NP is used.
> -
> -ESCAPE_ANY_NP is the sane choice for many cases, in particularly for
> -printing SSIDs.
> +This is used, for example, for printing SSIDs.
>  
>  If field width is omitted then 1 byte only will be escaped.
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..5522d2a052e1 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1537,9 +1537,6 @@ static noinline_for_stack
>  char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  		     const char *fmt)
>  {
> -	bool found = true;
> -	int count = 1;
> -	unsigned int flags = 0;
>  	int len;
>  
>  	if (spec.field_width == 0)
> @@ -1548,38 +1545,6 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  	if (check_pointer(&buf, end, addr, spec))
>  		return buf;
>  
> -	do {
> -		switch (fmt[count++]) {
> -		case 'a':
> -			flags |= ESCAPE_ANY;
> -			break;
> -		case 'c':
> -			flags |= ESCAPE_SPECIAL;
> -			break;
> -		case 'h':
> -			flags |= ESCAPE_HEX;
> -			break;
> -		case 'n':
> -			flags |= ESCAPE_NULL;
> -			break;
> -		case 'o':
> -			flags |= ESCAPE_OCTAL;
> -			break;
> -		case 'p':
> -			flags |= ESCAPE_NP;
> -			break;
> -		case 's':
> -			flags |= ESCAPE_SPACE;
> -			break;
> -		default:
> -			found = false;
> -			break;
> -		}
> -	} while (found);
> -
> -	if (!flags)
> -		flags = ESCAPE_ANY_NP;
> -
>  	len = spec.field_width < 0 ? 1 : spec.field_width;
>  
>  	/*
> @@ -1587,7 +1552,8 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  	 * the given buffer, and returns the total size of the output
>  	 * had the buffer been big enough.
>  	 */
> -	buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL);
> +	buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0,
> +				 ESCAPE_ANY_NP, NULL);
>  
>  	return buf;
>  }
> @@ -2038,17 +2004,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
>   * - '[Ii][4S][hnbl]' IPv4 addresses in host, network, big or little endian order
>   * - 'I[6S]c' for IPv6 addresses printed as specified by
>   *       http://tools.ietf.org/html/rfc5952
> - * - 'E[achnops]' For an escaped buffer, where rules are defined by combination
> - *                of the following flags (see string_escape_mem() for the
> - *                details):
> - *                  a - ESCAPE_ANY
> - *                  c - ESCAPE_SPECIAL
> - *                  h - ESCAPE_HEX
> - *                  n - ESCAPE_NULL
> - *                  o - ESCAPE_OCTAL
> - *                  p - ESCAPE_NP
> - *                  s - ESCAPE_SPACE
> - *                By default ESCAPE_ANY_NP is used.
> + * - 'E[achnops]' For an escaped buffer (see string_escape_mem()

This line should be like this; no more suboptions and add missed final ")":

 * - 'E' For an escaped buffer (see string_escape_mem())

-Kees

>   * - 'U' For a 16 byte UUID/GUID, it prints the UUID/GUID in the form
>   *       "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
>   *       Options for %pU are:
> -- 
> 2.21.0
> 

-- 
Kees Cook

Re: [PATCH 5/9] Remove unused %*pE[achnops] formats

[Andy Shevchenko]

On Thu, Sep 05, 2019 at 03:44:29PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> The [achnops] are confusing, and in practice the only one anyone seems
> to need is the bare %*pE.
> 
> I think some set of modifiers here might actually be useful, but the
> ones we have are confusing and unused, so let's just toss these out and
> then rethink what we might want to add back later.

Have you evaluated potential users of this API. Do they need anything of the
existing functionality?

mangle_path()
tomoyo_print_bprm()
tomoyo_scan_bprm()
tomoyo_environ()
tomoyo_encode2()
tomoyo_const_part_length()

Maybe there are more, I didn't check it carefully.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/9] Remove unused %*pE[achnops] formats

[Andy Shevchenko]

On Fri, Sep 06, 2019 at 01:01:41PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 05, 2019 at 03:44:29PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > The [achnops] are confusing, and in practice the only one anyone seems
> > to need is the bare %*pE.
> > 
> > I think some set of modifiers here might actually be useful, but the
> > ones we have are confusing and unused, so let's just toss these out and
> > then rethink what we might want to add back later.
> 
> Have you evaluated potential users of this API. Do they need anything of the
> existing functionality?
> 
> mangle_path()
> tomoyo_print_bprm()
> tomoyo_scan_bprm()
> tomoyo_environ()
> tomoyo_encode2()
> tomoyo_const_part_length()
> 
> Maybe there are more, I didn't check it carefully.

This is comment basically to the absent cover letter, means to the entire
series.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 6/9] Eliminate unused ESCAPE_NULL, ESCAPE_SPACE flags

[J. Bruce Fields]

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.

Note the trickiest part is actually the tests, and I still need to check
them.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

Re: [PATCH 6/9] Eliminate unused ESCAPE_NULL, ESCAPE_SPACE flags

[Kees Cook]

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

Re: [PATCH 6/9] Eliminate unused ESCAPE_NULL, ESCAPE_SPACE flags

[Andy Shevchenko]

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.
> 
> Note the trickiest part is actually the tests, and I still need to check
> them.

Currently this _tries_ to follow the shorthand character classes which is
established by tools. For example, "\s" = "[ \t\r\n\f]". Also it (almost?)
matches the counterpart, i.e. string_unescape() classes.

So, if we would need something else, perhaps better to do it in the separate
flags.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 7/9] Simplify string_escape_mem

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

string_escape_mem is harder to use than necessary:

	- ESCAPE_NP sounds like it means "escape nonprinting
	  characters", but actually means "do not escape printing
	  characters"
	- the use of the "only" string to limit the list of escaped
	  characters rather than supplement them is confusing and
	  unehlpful.

So:

	- use the "only" string to add to, rather than replace, the list
	  of characters to escape
	- separate flags into those that select which characters to
	  escape, and those that choose the format of the escaping ("\ "
	  vs "\x20" vs "\040".)  Make flags that select characters just
	  select the union when they're OR'd together.

Untested.  The tests themselves, especially, I'm unsure about.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/proc/array.c                |  2 +-
 fs/seq_file.c                  |  2 +-
 include/linux/string_helpers.h | 14 +++----
 lib/string_helpers.c           | 76 +++++++++++++++-------------------
 lib/test-string_helpers.c      | 41 ++++++++----------
 lib/vsprintf.c                 |  2 +-
 net/sunrpc/cache.c             |  2 +-
 7 files changed, 62 insertions(+), 77 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 982c95d09176..bdeeb3318fa2 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_SPECIAL, "\n\\");
+					ESCAPE_STYLE_SLASH, "\n\\");
 		if (ret >= size)
 			ret = -1;
 	} else {
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1600034a929b..63e5a7c4dbf7 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -379,7 +379,7 @@ void seq_escape(struct seq_file *m, const char *s, const char *esc)
 	size_t size = seq_get_buf(m, &buf);
 	int ret;
 
-	ret = string_escape_str(s, buf, size, ESCAPE_OCTAL, esc);
+	ret = string_escape_str(s, buf, size, ESCAPE_STYLE_OCTAL, esc);
 	seq_commit(m, ret < size ? ret : -1);
 }
 EXPORT_SYMBOL(seq_escape);
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 7bf0d137373d..5d350f7f6874 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -41,13 +41,13 @@ static inline int string_unescape_any_inplace(char *buf)
 	return string_unescape_any(buf, buf, 0);
 }
 
-#define ESCAPE_SPECIAL		0x02
-#define ESCAPE_OCTAL		0x08
-#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
+#define ESCAPE_SPECIAL		0x01
+#define ESCAPE_NP		0x02
+#define ESCAPE_ANY_NP		(ESCAPE_SPECIAL | ESCAPE_NP)
+#define ESCAPE_STYLE_SLASH	0x20
+#define ESCAPE_STYLE_OCTAL	0x40
+#define ESCAPE_STYLE_HEX	0x80
+#define ESCAPE_FLAG_MASK	0xe3
 
 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 ac72159d3980..47f40406f9d4 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -400,6 +400,11 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
 	return true;
 }
 
+static bool is_special(char c)
+{
+	return c == '\0' || strchr("\f\n\r\t\v\\\a\e", c);
+}
+
 /**
  * string_escape_mem - quote characters in the given memory buffer
  * @src:	source buffer (unescaped)
@@ -407,23 +412,18 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
  * @dst:	destination buffer (escaped)
  * @osz:	destination buffer size
  * @flags:	combination of the flags
- * @only:	NULL-terminated string containing characters used to limit
- *		the selected escape class. If characters are included in @only
- *		that would not normally be escaped by the classes selected
- *		in @flags, they will be copied to @dst unescaped.
+ * @esc:	NULL-terminated string containing characters which
+ *		should also be escaped.
  *
- * Description:
- * The process of escaping byte buffer includes several parts. They are applied
- * in the following sequence.
  *
- *	1. The character is matched to the printable class, if asked, and in
- *	   case of match it passes through to the output.
- *	2. The character is not matched to the one from @only string and thus
- *	   must go as-is to the output.
- *	3. The character is checked if it falls into the class given by @flags.
- *	   %ESCAPE_OCTAL and %ESCAPE_HEX are going last since they cover any
- *	   character. Note that they actually can't go together, otherwise
- *	   %ESCAPE_HEX will be ignored.
+ * Description:
+ * The characters selected by ESCAPE_* flags and by the "esc" string
+ * are escaped, using the escaping specified by the ESCAPE_STYLE_*
+ * flags.  If ESCAPE_STYLE_SLASH is specified together with one of the
+ * ESCAPE_STYLE_OCTAL or ESCAPE_STYLE_HEX flags, then those characters
+ * for which special slash escapes exist will be escaped using those,
+ * with others escaped using octal or hexidecimal values.  If
+ * unspecified, the default is ESCAPE_STYLE_OCTAL.
  *
  * Caller must provide valid source and destination pointers. Be aware that
  * destination buffer will not be NULL-terminated, thus caller have to append
@@ -439,14 +439,14 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
  *		'\a' - alert (BEL)
  *		'\e' - escape
  *		'\0' - null
- *	%ESCAPE_OCTAL:
- *		'\NNN' - byte with octal value NNN (3 digits)
- *	%ESCAPE_ANY:
- *		all previous together
  *	%ESCAPE_NP:
  *		escape only non-printable characters (checked by isprint)
  *	%ESCAPE_ANY_NP:
  *		all previous together
+ *	%ESCAPE_STYLE_SLASH:
+ *		Escape using the slash codes shown above
+ *	%ESCAPE_STYLE_OCTAL:
+ *		'\NNN' - byte with octal value NNN (3 digits)
  *	%ESCAPE_HEX:
  *		'\xHH' - byte with hexadecimal value HH (2 digits)
  *
@@ -457,39 +457,31 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
  * dst for a '\0' terminator if and only if ret < osz.
  */
 int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
-		      unsigned int flags, const char *only)
+		      unsigned int flags, const char *esc)
 {
 	char *p = dst;
 	char *end = p + osz;
-	bool is_dict = only && *only;
+	bool is_dict = esc && *esc;
 
 	while (isz--) {
 		unsigned char c = *src++;
 
-		/*
-		 * Apply rules in the following sequence:
-		 *	- the character is printable, when @flags has
-		 *	  %ESCAPE_NP bit set
-		 *	- the @only string is supplied and does not contain a
-		 *	  character under question
-		 *	- the character doesn't fall into a class of symbols
-		 *	  defined by given @flags
-		 * In these cases we just pass through a character to the
-		 * output buffer.
-		 */
-		if ((flags & ESCAPE_NP && isprint(c)) ||
-		    (is_dict && !strchr(only, c))) {
-			/* do nothing */
-		} else {
-			if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end))
-				continue;
+		if ((is_dict && strchr(esc, c)) ||
+		    (flags & ESCAPE_NP && !isprint(c)) ||
+		    (flags & ESCAPE_SPECIAL && is_special(c))) {
 
-			/* ESCAPE_OCTAL and ESCAPE_HEX always go last */
-			if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end))
+			if (flags & ESCAPE_STYLE_SLASH &&
+					escape_special(c, &p, end))
 				continue;
 
-			if (flags & ESCAPE_HEX && escape_hex(c, &p, end))
+			if (flags & ESCAPE_STYLE_HEX &&
+					escape_hex(c, &p, end))
 				continue;
+			/*
+			 * Always the default, so a caller doesn't
+			 * accidentally leave something unescaped:
+			 */
+			escape_octal(c, &p, end);
 		}
 
 		escape_passthrough(c, &p, end);
@@ -526,7 +518,7 @@ char *kstrdup_quotable(const char *src, gfp_t gfp)
 {
 	size_t slen, dlen;
 	char *dst;
-	const int flags = ESCAPE_HEX;
+	const int flags = ESCAPE_STYLE_HEX;
 	const char esc[] = "\f\n\r\t\v\a\e\\\"";
 
 	if (!src)
diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
index 0da3c195a327..d40fedab24ad 100644
--- a/lib/test-string_helpers.c
+++ b/lib/test-string_helpers.c
@@ -127,13 +127,13 @@ static const struct test_string_2 escape0[] __initconst = {{
 	.in = "\\h\\\"\a\e\\",
 	.s1 = {{
 		.out = "\\\\h\\\\\"\\a\\e\\\\",
-		.flags = ESCAPE_SPECIAL,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH,
 	},{
 		.out = "\\\\\\150\\\\\\042\\a\\e\\\\",
-		.flags = ESCAPE_SPECIAL | ESCAPE_OCTAL,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_OCTAL,
 	},{
 		.out = "\\\\\\x68\\\\\\x22\\a\\e\\\\",
-		.flags = ESCAPE_SPECIAL | ESCAPE_HEX,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_HEX,
 	},{
 		/* terminator */
 	}},
@@ -141,28 +141,19 @@ static const struct test_string_2 escape0[] __initconst = {{
 	.in = "\eb \\C\007\"\x90\r]",
 	.s1 = {{
 		.out = "\\eb \\\\C\\a\"\x90\\r]",
-		.flags = ESCAPE_SPECIAL,
-	},{
-		.out = "\\033\\142\\040\\134\\103\\007\\042\\220\\015\\135",
-		.flags = ESCAPE_OCTAL,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH,
 	},{
 		.out = "\\e\\142\\040\\\\\\103\\a\\042\\220\\r\\135",
-		.flags = ESCAPE_SPECIAL | ESCAPE_OCTAL,
-	},{
-		.out = "\eb \\C\007\"\x90\r]",
-		.flags = ESCAPE_NP,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_OCTAL,
 	},{
 		.out = "\\eb \\C\\a\"\x90\\r]",
-		.flags = ESCAPE_SPECIAL | ESCAPE_NP,
-	},{
-		.out = "\\033b \\C\\007\"\\220\\015]",
-		.flags = ESCAPE_OCTAL | ESCAPE_NP,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_OCTAL,
 	},{
 		.out = "\\eb \\C\\a\"\\220\\r]",
-		.flags = ESCAPE_SPECIAL ESCAPE_OCTAL | ESCAPE_NP,
+		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_OCTAL | ESCAPE_NP,
 	},{
 		.out = "\\x1bb \\C\\x07\"\\x90\\x0d]",
-		.flags = ESCAPE_NP | ESCAPE_HEX,
+		.flags = ESCAPE_NP | ESCAPE_STYLE_HEX,
 	},{
 		/* terminator */
 	}},
@@ -175,10 +166,10 @@ static const struct test_string_2 escape1[] __initconst = {{
 	.in = "\f\\ \n\r\t\v",
 	.s1 = {{
 		.out = "\f\\134\\040\n\\015\\011\v",
-		.flags = ESCAPE_OCTAL,
+		.flags = ESCAPE_STYLE_OCTAL,
 	},{
 		.out = "\f\\x5c\\x20\n\\x0d\\x09\v",
-		.flags = ESCAPE_HEX,
+		.flags = ESCAPE_STYLE_HEX,
 	},{
 		/* terminator */
 	}},
@@ -186,7 +177,7 @@ static const struct test_string_2 escape1[] __initconst = {{
 	.in = "\\h\\\"\a\e\\",
 	.s1 = {{
 		.out = "\\134h\\134\"\a\e\\134",
-		.flags = ESCAPE_OCTAL,
+		.flags = ESCAPE_STYLE_OCTAL,
 	},{
 		/* terminator */
 	}},
@@ -194,7 +185,7 @@ static const struct test_string_2 escape1[] __initconst = {{
 	.in = "\eb \\C\007\"\x90\r]",
 	.s1 = {{
 		.out = "\e\\142\\040\\134C\007\"\x90\\015]",
-		.flags = ESCAPE_OCTAL,
+		.flags = ESCAPE_STYLE_OCTAL,
 	},{
 		/* terminator */
 	}},
@@ -211,9 +202,9 @@ static __init const char *test_string_find_match(const struct test_string_2 *s2,
 	if (!flags)
 		return s2->in;
 
-	/* ESCAPE_OCTAL has a higher priority */
-	if (flags & ESCAPE_OCTAL)
-		flags &= ~ESCAPE_HEX;
+	/* ESCAPE_OCTAL is default: */
+	if (!(flags & ESCAPE_STYLE_HEX))
+		flags |= ESCAPE_STYLE_OCTAL;
 
 	for (i = 0; i < TEST_STRING_2_MAX_S1 && s1->out; i++, s1++)
 		if (s1->flags == flags)
@@ -266,6 +257,8 @@ static __init void test_string_escape(const char *name,
 		memcpy(&out_test[q_test], out, len);
 		q_test += len;
 	}
+	if (!p)
+		goto out;
 
 	q_real = string_escape_mem(in, p, out_real, out_size, flags, esc);
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 5522d2a052e1..020aff0c3fd9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1553,7 +1553,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	 * had the buffer been big enough.
 	 */
 	buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0,
-				 ESCAPE_ANY_NP, NULL);
+				 ESCAPE_ANY_NP|ESCAPE_STYLE_SLASH, NULL);
 
 	return buf;
 }
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 6f1528f271ee..1b5a2b9e7808 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -1118,7 +1118,7 @@ void qword_add(char **bpp, int *lp, char *str)
 
 	if (len < 0) return;
 
-	ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t");
+	ret = string_escape_str(str, bp, len, ESCAPE_STYLE_OCTAL, "\\ \n\t");
 	if (ret >= len) {
 		bp += len;
 		len = -1;
-- 
2.21.0

Re: [PATCH 7/9] Simplify string_escape_mem

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:31PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> string_escape_mem is harder to use than necessary:
> 
> 	- ESCAPE_NP sounds like it means "escape nonprinting
> 	  characters", but actually means "do not escape printing
> 	  characters"
> 	- the use of the "only" string to limit the list of escaped
> 	  characters rather than supplement them is confusing and
> 	  unehlpful.
> 
> So:
> 
> 	- use the "only" string to add to, rather than replace, the list
> 	  of characters to escape
> 	- separate flags into those that select which characters to
> 	  escape, and those that choose the format of the escaping ("\ "
> 	  vs "\x20" vs "\040".)  Make flags that select characters just
> 	  select the union when they're OR'd together.
> 
> Untested.  The tests themselves, especially, I'm unsure about.

I'll take a look at these too. Notes below...

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/proc/array.c                |  2 +-
>  fs/seq_file.c                  |  2 +-
>  include/linux/string_helpers.h | 14 +++----
>  lib/string_helpers.c           | 76 +++++++++++++++-------------------
>  lib/test-string_helpers.c      | 41 ++++++++----------
>  lib/vsprintf.c                 |  2 +-
>  net/sunrpc/cache.c             |  2 +-
>  7 files changed, 62 insertions(+), 77 deletions(-)
> 
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 982c95d09176..bdeeb3318fa2 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_SPECIAL, "\n\\");
> +					ESCAPE_STYLE_SLASH, "\n\\");
>  		if (ret >= size)
>  			ret = -1;
>  	} else {
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1600034a929b..63e5a7c4dbf7 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -379,7 +379,7 @@ void seq_escape(struct seq_file *m, const char *s, const char *esc)
>  	size_t size = seq_get_buf(m, &buf);
>  	int ret;
>  
> -	ret = string_escape_str(s, buf, size, ESCAPE_OCTAL, esc);
> +	ret = string_escape_str(s, buf, size, ESCAPE_STYLE_OCTAL, esc);
>  	seq_commit(m, ret < size ? ret : -1);
>  }
>  EXPORT_SYMBOL(seq_escape);
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 7bf0d137373d..5d350f7f6874 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -41,13 +41,13 @@ static inline int string_unescape_any_inplace(char *buf)
>  	return string_unescape_any(buf, buf, 0);
>  }
>  
> -#define ESCAPE_SPECIAL		0x02
> -#define ESCAPE_OCTAL		0x08
> -#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
> +#define ESCAPE_SPECIAL		0x01
> +#define ESCAPE_NP		0x02
> +#define ESCAPE_ANY_NP		(ESCAPE_SPECIAL | ESCAPE_NP)
> +#define ESCAPE_STYLE_SLASH	0x20
> +#define ESCAPE_STYLE_OCTAL	0x40
> +#define ESCAPE_STYLE_HEX	0x80
> +#define ESCAPE_FLAG_MASK	0xe3
>  
>  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 ac72159d3980..47f40406f9d4 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -400,6 +400,11 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
>  	return true;
>  }
>  
> +static bool is_special(char c)
> +{
> +	return c == '\0' || strchr("\f\n\r\t\v\\\a\e", c);
> +}
> +
>  /**
>   * string_escape_mem - quote characters in the given memory buffer
>   * @src:	source buffer (unescaped)
> @@ -407,23 +412,18 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
>   * @dst:	destination buffer (escaped)
>   * @osz:	destination buffer size
>   * @flags:	combination of the flags
> - * @only:	NULL-terminated string containing characters used to limit
> - *		the selected escape class. If characters are included in @only
> - *		that would not normally be escaped by the classes selected
> - *		in @flags, they will be copied to @dst unescaped.
> + * @esc:	NULL-terminated string containing characters which
> + *		should also be escaped.
>   *
> - * Description:
> - * The process of escaping byte buffer includes several parts. They are applied
> - * in the following sequence.
>   *
> - *	1. The character is matched to the printable class, if asked, and in
> - *	   case of match it passes through to the output.
> - *	2. The character is not matched to the one from @only string and thus
> - *	   must go as-is to the output.
> - *	3. The character is checked if it falls into the class given by @flags.
> - *	   %ESCAPE_OCTAL and %ESCAPE_HEX are going last since they cover any
> - *	   character. Note that they actually can't go together, otherwise
> - *	   %ESCAPE_HEX will be ignored.
> + * Description:
> + * The characters selected by ESCAPE_* flags and by the "esc" string
> + * are escaped, using the escaping specified by the ESCAPE_STYLE_*
> + * flags.  If ESCAPE_STYLE_SLASH is specified together with one of the
> + * ESCAPE_STYLE_OCTAL or ESCAPE_STYLE_HEX flags, then those characters
> + * for which special slash escapes exist will be escaped using those,
> + * with others escaped using octal or hexidecimal values.  If
> + * unspecified, the default is ESCAPE_STYLE_OCTAL.
>   *
>   * Caller must provide valid source and destination pointers. Be aware that
>   * destination buffer will not be NULL-terminated, thus caller have to append
> @@ -439,14 +439,14 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
>   *		'\a' - alert (BEL)
>   *		'\e' - escape
>   *		'\0' - null
> - *	%ESCAPE_OCTAL:
> - *		'\NNN' - byte with octal value NNN (3 digits)
> - *	%ESCAPE_ANY:
> - *		all previous together
>   *	%ESCAPE_NP:
>   *		escape only non-printable characters (checked by isprint)
>   *	%ESCAPE_ANY_NP:
>   *		all previous together
> + *	%ESCAPE_STYLE_SLASH:
> + *		Escape using the slash codes shown above
> + *	%ESCAPE_STYLE_OCTAL:
> + *		'\NNN' - byte with octal value NNN (3 digits)
>   *	%ESCAPE_HEX:
>   *		'\xHH' - byte with hexadecimal value HH (2 digits)
>   *
> @@ -457,39 +457,31 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
>   * dst for a '\0' terminator if and only if ret < osz.
>   */
>  int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
> -		      unsigned int flags, const char *only)
> +		      unsigned int flags, const char *esc)
>  {
>  	char *p = dst;
>  	char *end = p + osz;
> -	bool is_dict = only && *only;
> +	bool is_dict = esc && *esc;
>  
>  	while (isz--) {
>  		unsigned char c = *src++;
>  
> -		/*
> -		 * Apply rules in the following sequence:
> -		 *	- the character is printable, when @flags has
> -		 *	  %ESCAPE_NP bit set
> -		 *	- the @only string is supplied and does not contain a
> -		 *	  character under question
> -		 *	- the character doesn't fall into a class of symbols
> -		 *	  defined by given @flags
> -		 * In these cases we just pass through a character to the
> -		 * output buffer.
> -		 */
> -		if ((flags & ESCAPE_NP && isprint(c)) ||
> -		    (is_dict && !strchr(only, c))) {
> -			/* do nothing */
> -		} else {
> -			if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end))
> -				continue;
> +		if ((is_dict && strchr(esc, c)) ||
> +		    (flags & ESCAPE_NP && !isprint(c)) ||
> +		    (flags & ESCAPE_SPECIAL && is_special(c))) {

One thing that needs fixing a bit here is the handling of '\' in input
strings. Anything that does escaping must escape '\' as well otherwise
output string encodings can be spoofed. e.g. input of "\100" doing
non-printable escaping will not escape anything, but anything trying to
reconstruct the input from the output will process it as "@" (octal
100). So "\" must always be escaped (and I think this was a bug in the
old handling too.) So I think it likely needs to be explicitly included,
maybe like this:

		if ((is_dict && strchr(esc, c)) || c == '\' ||
		    (flags & ESCAPE_NP && !isprint(c)) ||
		    (flags & ESCAPE_SPECIAL && is_special(c))) {

Or maybe a mask can be used to check "flags" for non-style escape flags
being requested and if any are set, make sure '\' is included.

-Kees

>  
> -			/* ESCAPE_OCTAL and ESCAPE_HEX always go last */
> -			if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end))
> +			if (flags & ESCAPE_STYLE_SLASH &&
> +					escape_special(c, &p, end))
>  				continue;
>  
> -			if (flags & ESCAPE_HEX && escape_hex(c, &p, end))
> +			if (flags & ESCAPE_STYLE_HEX &&
> +					escape_hex(c, &p, end))
>  				continue;
> +			/*
> +			 * Always the default, so a caller doesn't
> +			 * accidentally leave something unescaped:
> +			 */
> +			escape_octal(c, &p, end);
>  		}
>  
>  		escape_passthrough(c, &p, end);
> @@ -526,7 +518,7 @@ char *kstrdup_quotable(const char *src, gfp_t gfp)
>  {
>  	size_t slen, dlen;
>  	char *dst;
> -	const int flags = ESCAPE_HEX;
> +	const int flags = ESCAPE_STYLE_HEX;
>  	const char esc[] = "\f\n\r\t\v\a\e\\\"";
>  
>  	if (!src)
> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
> index 0da3c195a327..d40fedab24ad 100644
> --- a/lib/test-string_helpers.c
> +++ b/lib/test-string_helpers.c
> @@ -127,13 +127,13 @@ static const struct test_string_2 escape0[] __initconst = {{
>  	.in = "\\h\\\"\a\e\\",
>  	.s1 = {{
>  		.out = "\\\\h\\\\\"\\a\\e\\\\",
> -		.flags = ESCAPE_SPECIAL,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH,
>  	},{
>  		.out = "\\\\\\150\\\\\\042\\a\\e\\\\",
> -		.flags = ESCAPE_SPECIAL | ESCAPE_OCTAL,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_OCTAL,
>  	},{
>  		.out = "\\\\\\x68\\\\\\x22\\a\\e\\\\",
> -		.flags = ESCAPE_SPECIAL | ESCAPE_HEX,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_HEX,
>  	},{
>  		/* terminator */
>  	}},
> @@ -141,28 +141,19 @@ static const struct test_string_2 escape0[] __initconst = {{
>  	.in = "\eb \\C\007\"\x90\r]",
>  	.s1 = {{
>  		.out = "\\eb \\\\C\\a\"\x90\\r]",
> -		.flags = ESCAPE_SPECIAL,
> -	},{
> -		.out = "\\033\\142\\040\\134\\103\\007\\042\\220\\015\\135",
> -		.flags = ESCAPE_OCTAL,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH,
>  	},{
>  		.out = "\\e\\142\\040\\\\\\103\\a\\042\\220\\r\\135",
> -		.flags = ESCAPE_SPECIAL | ESCAPE_OCTAL,
> -	},{
> -		.out = "\eb \\C\007\"\x90\r]",
> -		.flags = ESCAPE_NP,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_OCTAL,
>  	},{
>  		.out = "\\eb \\C\\a\"\x90\\r]",
> -		.flags = ESCAPE_SPECIAL | ESCAPE_NP,
> -	},{
> -		.out = "\\033b \\C\\007\"\\220\\015]",
> -		.flags = ESCAPE_OCTAL | ESCAPE_NP,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_SLASH | ESCAPE_STYLE_OCTAL,
>  	},{
>  		.out = "\\eb \\C\\a\"\\220\\r]",
> -		.flags = ESCAPE_SPECIAL ESCAPE_OCTAL | ESCAPE_NP,
> +		.flags = ESCAPE_SPECIAL | ESCAPE_STYLE_OCTAL | ESCAPE_NP,
>  	},{
>  		.out = "\\x1bb \\C\\x07\"\\x90\\x0d]",
> -		.flags = ESCAPE_NP | ESCAPE_HEX,
> +		.flags = ESCAPE_NP | ESCAPE_STYLE_HEX,
>  	},{
>  		/* terminator */
>  	}},
> @@ -175,10 +166,10 @@ static const struct test_string_2 escape1[] __initconst = {{
>  	.in = "\f\\ \n\r\t\v",
>  	.s1 = {{
>  		.out = "\f\\134\\040\n\\015\\011\v",
> -		.flags = ESCAPE_OCTAL,
> +		.flags = ESCAPE_STYLE_OCTAL,
>  	},{
>  		.out = "\f\\x5c\\x20\n\\x0d\\x09\v",
> -		.flags = ESCAPE_HEX,
> +		.flags = ESCAPE_STYLE_HEX,
>  	},{
>  		/* terminator */
>  	}},
> @@ -186,7 +177,7 @@ static const struct test_string_2 escape1[] __initconst = {{
>  	.in = "\\h\\\"\a\e\\",
>  	.s1 = {{
>  		.out = "\\134h\\134\"\a\e\\134",
> -		.flags = ESCAPE_OCTAL,
> +		.flags = ESCAPE_STYLE_OCTAL,
>  	},{
>  		/* terminator */
>  	}},
> @@ -194,7 +185,7 @@ static const struct test_string_2 escape1[] __initconst = {{
>  	.in = "\eb \\C\007\"\x90\r]",
>  	.s1 = {{
>  		.out = "\e\\142\\040\\134C\007\"\x90\\015]",
> -		.flags = ESCAPE_OCTAL,
> +		.flags = ESCAPE_STYLE_OCTAL,
>  	},{
>  		/* terminator */
>  	}},
> @@ -211,9 +202,9 @@ static __init const char *test_string_find_match(const struct test_string_2 *s2,
>  	if (!flags)
>  		return s2->in;
>  
> -	/* ESCAPE_OCTAL has a higher priority */
> -	if (flags & ESCAPE_OCTAL)
> -		flags &= ~ESCAPE_HEX;
> +	/* ESCAPE_OCTAL is default: */
> +	if (!(flags & ESCAPE_STYLE_HEX))
> +		flags |= ESCAPE_STYLE_OCTAL;
>  
>  	for (i = 0; i < TEST_STRING_2_MAX_S1 && s1->out; i++, s1++)
>  		if (s1->flags == flags)
> @@ -266,6 +257,8 @@ static __init void test_string_escape(const char *name,
>  		memcpy(&out_test[q_test], out, len);
>  		q_test += len;
>  	}
> +	if (!p)
> +		goto out;
>  
>  	q_real = string_escape_mem(in, p, out_real, out_size, flags, esc);
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 5522d2a052e1..020aff0c3fd9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1553,7 +1553,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  	 * had the buffer been big enough.
>  	 */
>  	buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0,
> -				 ESCAPE_ANY_NP, NULL);
> +				 ESCAPE_ANY_NP|ESCAPE_STYLE_SLASH, NULL);
>  
>  	return buf;
>  }
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 6f1528f271ee..1b5a2b9e7808 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1118,7 +1118,7 @@ void qword_add(char **bpp, int *lp, char *str)
>  
>  	if (len < 0) return;
>  
> -	ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t");
> +	ret = string_escape_str(str, bp, len, ESCAPE_STYLE_OCTAL, "\\ \n\t");
>  	if (ret >= len) {
>  		bp += len;
>  		len = -1;
> -- 
> 2.21.0
> 

-- 
Kees Cook

[PATCH 8/9] minor kstrdup_quotable simplification

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

---
 lib/string_helpers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 47f40406f9d4..6f553f893fda 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -518,8 +518,8 @@ char *kstrdup_quotable(const char *src, gfp_t gfp)
 {
 	size_t slen, dlen;
 	char *dst;
-	const int flags = ESCAPE_STYLE_HEX;
-	const char esc[] = "\f\n\r\t\v\a\e\\\"";
+	const int flags = ESCAPE_SPECIAL|ESCAPE_STYLE_HEX;
+	const char esc[] = "\"";
 
 	if (!src)
 		return NULL;
-- 
2.21.0

Re: [PATCH 8/9] minor kstrdup_quotable simplification

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:32PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> ---
>  lib/string_helpers.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I think this can be folded into patch 7.

> 
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 47f40406f9d4..6f553f893fda 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -518,8 +518,8 @@ char *kstrdup_quotable(const char *src, gfp_t gfp)
>  {
>  	size_t slen, dlen;
>  	char *dst;
> -	const int flags = ESCAPE_STYLE_HEX;
> -	const char esc[] = "\f\n\r\t\v\a\e\\\"";
> +	const int flags = ESCAPE_SPECIAL|ESCAPE_STYLE_HEX;
> +	const char esc[] = "\"";
>  
>  	if (!src)
>  		return NULL;
> -- 
> 2.21.0
> 

-- 
Kees Cook

[PATCH 9/9] Remove string_escape_mem_ascii

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

It's easier to do this in string_escape_mem now.

Might also consider non-ascii and quote-mark sprintf modifiers and then
we might make do with seq_printk.
---
 fs/seq_file.c                  |  3 ++-
 include/linux/string_helpers.h |  3 +--
 lib/string_helpers.c           | 24 ++++--------------------
 3 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 63e5a7c4dbf7..0e45a25523ad 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -390,7 +390,8 @@ void seq_escape_mem_ascii(struct seq_file *m, const char *src, size_t isz)
 	size_t size = seq_get_buf(m, &buf);
 	int ret;
 
-	ret = string_escape_mem_ascii(src, isz, buf, size);
+	ret = string_escape_mem(src, isz, buf, size, ESCAPE_NP|ESCAPE_NONASCII|
+				ESCAPE_STYLE_SLASH|ESCAPE_STYLE_HEX, "\"\\");
 	seq_commit(m, ret < size ? ret : -1);
 }
 EXPORT_SYMBOL(seq_escape_mem_ascii);
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 5d350f7f6874..f3388591d83f 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -43,6 +43,7 @@ static inline int string_unescape_any_inplace(char *buf)
 
 #define ESCAPE_SPECIAL		0x01
 #define ESCAPE_NP		0x02
+#define ESCAPE_NONASCII		0x04
 #define ESCAPE_ANY_NP		(ESCAPE_SPECIAL | ESCAPE_NP)
 #define ESCAPE_STYLE_SLASH	0x20
 #define ESCAPE_STYLE_OCTAL	0x40
@@ -52,8 +53,6 @@ static inline int string_unescape_any_inplace(char *buf)
 int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 		unsigned int flags, const char *only);
 
-int string_escape_mem_ascii(const char *src, size_t isz, char *dst,
-					size_t osz);
 static inline int string_escape_str(const char *src, char *dst, size_t sz,
 		unsigned int flags, const char *only)
 {
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 6f553f893fda..1dacf76eada0 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -439,6 +439,8 @@ static bool is_special(char c)
  *		'\a' - alert (BEL)
  *		'\e' - escape
  *		'\0' - null
+ *	%ESCAPE_NONASCII:
+ *		escape characters with the high bit set
  *	%ESCAPE_NP:
  *		escape only non-printable characters (checked by isprint)
  *	%ESCAPE_ANY_NP:
@@ -468,7 +470,8 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 
 		if ((is_dict && strchr(esc, c)) ||
 		    (flags & ESCAPE_NP && !isprint(c)) ||
-		    (flags & ESCAPE_SPECIAL && is_special(c))) {
+		    (flags & ESCAPE_SPECIAL && is_special(c)) ||
+		    (flags & ESCAPE_NONASCII && !isascii(c))) {
 
 			if (flags & ESCAPE_STYLE_SLASH &&
 					escape_special(c, &p, end))
@@ -491,25 +494,6 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 }
 EXPORT_SYMBOL(string_escape_mem);
 
-int string_escape_mem_ascii(const char *src, size_t isz, char *dst,
-					size_t osz)
-{
-	char *p = dst;
-	char *end = p + osz;
-
-	while (isz--) {
-		unsigned char c = *src++;
-
-		if (!isprint(c) || !isascii(c) || c == '"' || c == '\\')
-			escape_hex(c, &p, end);
-		else
-			escape_passthrough(c, &p, end);
-	}
-
-	return p - dst;
-}
-EXPORT_SYMBOL(string_escape_mem_ascii);
-
 /*
  * Return an allocated string that has been escaped of special characters
  * and double quotes, making it safe to log in quotes.
-- 
2.21.0

Re: [PATCH 9/9] Remove string_escape_mem_ascii

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:33PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> It's easier to do this in string_escape_mem now.
> 
> Might also consider non-ascii and quote-mark sprintf modifiers and then
> we might make do with seq_printk.

With '\' always handled, it can be dropped from the "esc" args below. I
wonder if ESCAPE_QUOTES is needed or if "esc" can just continue to be
used for that.

(Also, do we want to add the "hex escape" modifier back to snprintf's
%pE stuff?)

-Kees

> ---
>  fs/seq_file.c                  |  3 ++-
>  include/linux/string_helpers.h |  3 +--
>  lib/string_helpers.c           | 24 ++++--------------------
>  3 files changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 63e5a7c4dbf7..0e45a25523ad 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -390,7 +390,8 @@ void seq_escape_mem_ascii(struct seq_file *m, const char *src, size_t isz)
>  	size_t size = seq_get_buf(m, &buf);
>  	int ret;
>  
> -	ret = string_escape_mem_ascii(src, isz, buf, size);
> +	ret = string_escape_mem(src, isz, buf, size, ESCAPE_NP|ESCAPE_NONASCII|
> +				ESCAPE_STYLE_SLASH|ESCAPE_STYLE_HEX, "\"\\");
>  	seq_commit(m, ret < size ? ret : -1);
>  }
>  EXPORT_SYMBOL(seq_escape_mem_ascii);
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 5d350f7f6874..f3388591d83f 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -43,6 +43,7 @@ static inline int string_unescape_any_inplace(char *buf)
>  
>  #define ESCAPE_SPECIAL		0x01
>  #define ESCAPE_NP		0x02
> +#define ESCAPE_NONASCII		0x04
>  #define ESCAPE_ANY_NP		(ESCAPE_SPECIAL | ESCAPE_NP)
>  #define ESCAPE_STYLE_SLASH	0x20
>  #define ESCAPE_STYLE_OCTAL	0x40
> @@ -52,8 +53,6 @@ static inline int string_unescape_any_inplace(char *buf)
>  int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
>  		unsigned int flags, const char *only);
>  
> -int string_escape_mem_ascii(const char *src, size_t isz, char *dst,
> -					size_t osz);
>  static inline int string_escape_str(const char *src, char *dst, size_t sz,
>  		unsigned int flags, const char *only)
>  {
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 6f553f893fda..1dacf76eada0 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -439,6 +439,8 @@ static bool is_special(char c)
>   *		'\a' - alert (BEL)
>   *		'\e' - escape
>   *		'\0' - null
> + *	%ESCAPE_NONASCII:
> + *		escape characters with the high bit set
>   *	%ESCAPE_NP:
>   *		escape only non-printable characters (checked by isprint)
>   *	%ESCAPE_ANY_NP:
> @@ -468,7 +470,8 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
>  
>  		if ((is_dict && strchr(esc, c)) ||
>  		    (flags & ESCAPE_NP && !isprint(c)) ||
> -		    (flags & ESCAPE_SPECIAL && is_special(c))) {
> +		    (flags & ESCAPE_SPECIAL && is_special(c)) ||
> +		    (flags & ESCAPE_NONASCII && !isascii(c))) {
>  
>  			if (flags & ESCAPE_STYLE_SLASH &&
>  					escape_special(c, &p, end))
> @@ -491,25 +494,6 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
>  }
>  EXPORT_SYMBOL(string_escape_mem);
>  
> -int string_escape_mem_ascii(const char *src, size_t isz, char *dst,
> -					size_t osz)
> -{
> -	char *p = dst;
> -	char *end = p + osz;
> -
> -	while (isz--) {
> -		unsigned char c = *src++;
> -
> -		if (!isprint(c) || !isascii(c) || c == '"' || c == '\\')
> -			escape_hex(c, &p, end);
> -		else
> -			escape_passthrough(c, &p, end);
> -	}
> -
> -	return p - dst;
> -}
> -EXPORT_SYMBOL(string_escape_mem_ascii);
> -
>  /*
>   * Return an allocated string that has been escaped of special characters
>   * and double quotes, making it safe to log in quotes.
> -- 
> 2.21.0
> 

-- 
Kees Cook

Re: [PATCH 9/9] Remove string_escape_mem_ascii

[Andy Shevchenko]

On Thu, Sep 05, 2019 at 03:44:33PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> It's easier to do this in string_escape_mem now.
> 
> Might also consider non-ascii and quote-mark sprintf modifiers and then
> we might make do with seq_printk.

No SoB :-)
Entire series forgot to include RFC prefix.

Nevertheless, this one I like independently on what we decide about flags.

> -	ret = string_escape_mem_ascii(src, isz, buf, size);
> +	ret = string_escape_mem(src, isz, buf, size, ESCAPE_NP|ESCAPE_NONASCII|
> +				ESCAPE_STYLE_SLASH|ESCAPE_STYLE_HEX, "\"\\");

Perhaps,

	unsigned int flags = FLAG1 | FLAG2 ...;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] rtl8192*: display ESSIDs using %pE

[Kees Cook]

On Thu, Sep 05, 2019 at 03:44:25PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Everywhere else in the kernel ESSIDs are printed using %pE, and I can't
> see why there should be an exception here.

I would expand this rationale slightly: using "n" here makes no sense
because they are already NUL-terminated strings. The "n" modifier could
only be used with string_escape_mem() which takes a "length" argument.

Regardless:

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  drivers/staging/rtl8192e/rtllib.h              | 2 +-
>  drivers/staging/rtl8192u/ieee80211/ieee80211.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
> index 2dd57e88276e..096254e422b3 100644
> --- a/drivers/staging/rtl8192e/rtllib.h
> +++ b/drivers/staging/rtl8192e/rtllib.h
> @@ -2132,7 +2132,7 @@ static inline const char *escape_essid(const char *essid, u8 essid_len)
>  		return escaped;
>  	}
>  
> -	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
> +	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);
>  	return escaped;
>  }
>  
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> index d36963469015..3963a08b9eb2 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> @@ -2426,7 +2426,7 @@ static inline const char *escape_essid(const char *essid, u8 essid_len)
>  		return escaped;
>  	}
>  
> -	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
> +	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);
>  	return escaped;
>  }
>  
> -- 
> 2.21.0
> 

-- 
Kees Cook

Re: [PATCH 1/9] rtl8192*: display ESSIDs using %pE

[Andy Shevchenko]

On Thu, Sep 05, 2019 at 01:53:43PM -0700, Kees Cook wrote:
> On Thu, Sep 05, 2019 at 03:44:25PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > Everywhere else in the kernel ESSIDs are printed using %pE, and I can't
> > see why there should be an exception here.
> 
> I would expand this rationale slightly: using "n" here makes no sense
> because they are already NUL-terminated strings. The "n" modifier could
> only be used with string_escape_mem() which takes a "length" argument.

SSID may have NUL in any location in the name.

> > -	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
> > +	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);

> > -	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
> > +	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] rtl8192*: display ESSIDs using %pE

[Kees Cook]

On Fri, Sep 06, 2019 at 12:38:29PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 05, 2019 at 01:53:43PM -0700, Kees Cook wrote:
> > On Thu, Sep 05, 2019 at 03:44:25PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > 
> > > Everywhere else in the kernel ESSIDs are printed using %pE, and I can't
> > > see why there should be an exception here.
> > 
> > I would expand this rationale slightly: using "n" here makes no sense
> > because they are already NUL-terminated strings. The "n" modifier could
> > only be used with string_escape_mem() which takes a "length" argument.
> 
> SSID may have NUL in any location in the name.

Oops, you're totally right: I forgot the "*" part here. Ignore my
comment. :)

So, instead, this "upgrades" the escaping from "only NULL" to all the
unprintables.

> 
> > > -	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
> > > +	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);
> 
> > > -	snprintf(escaped, sizeof(escaped), "%*pEn", essid_len, essid);
> > > +	snprintf(escaped, sizeof(escaped), "%*pE", essid_len, essid);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
Kees Cook