[PATCH 1/4] string: try to find const-laundering bugs

[PATCH 1/4] string: try to find const-laundering bugs

[Rasmus Villemoes]

This wraps strchr and friends in macros that ensure the return value has
type const char* if the passed-in string (which the return value points
into) also has type const char*. The (s)+0 thing is to force a const
char[] (e.g. a string literal) to decay to a const char* for the
__same_type comparison.

Not sure it's worth the churn necessary to eliminate the resulting
warnings, but maybe it'll find some actual bugs. It certainly spews
out lots of warnings all over the tree, but instead of trying to fix
them up before actually getting this patch in, I've made the use of
these macros opt-in so anybody can do a build with
KCFLAGS=-D__CONST_LAUNDER (or add a 'ccflags-y += ...' to some
sub-Makefile) and take a look at their subsystem of interest.

Note that the use of the identifier strchr in the macro strchr is not a
problem; the preprocessor rules say that the expanded strchr should be
left alone. But we do have to prevent macro expansion when actually
defining the strchr function, hence the lib/string.c part of this. For
the same reason, I'm not defining strchr when __HAVE_ARCH_STRCHR, since
then I'd have to go into all those arches and protect their definitions
similarly.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Patches 2,3,4 can be applied independently of each other and this one,
and just serve as examples of the kind of churn enabling these
unconditionally would give.

While playing with this, I haven't found any obvious bugs, but one
example where it's not immediately obvious whether there is a problem
is

	comma = strchr(dev_name, ',');
	if (comma != NULL && comma < end)
		*comma = 0;

in fs/nfs/super.c. dev_name is const char*, but it is modified through
comma. I haven't been able to track back through all the VFS callbacks
whether that's perfectly fine. At least a comment would be nice.

 include/linux/string.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/string.c           | 12 ++++++------
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 4a5a0eb7df51..06d260cdc4b7 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -449,4 +449,51 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
 		memcpy(dest, src, dest_len);
 }
 
+#ifdef __CONST_LAUNDER
+
+#ifndef __HAVE_ARCH_STRCHR
+#define strchr(s, c) (						  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strchr(s, c),	  \
+			      strchr(s, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRCHRNUL
+#define strchrnul(s, c) (					  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strchrnul(s, c),	  \
+			      strchrnul(s, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRNCHR
+#define strnchr(s, n, c) (					  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strnchr(s, n, c),	  \
+			      strnchr(s, n, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRRCHR
+#define strrchr(s, c) (						  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strrchr(s, c),	  \
+			      strrchr(s, c)))
+#endif
+
+#ifndef __HAVE_ARCH_STRSTR
+#define strstr(s, t) (						  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strstr(s, t),	  \
+			      strstr(s, t)))
+#endif
+
+#ifndef __HAVE_ARCH_STRNSTR
+#define strnstr(s, t, n) (					  \
+	__builtin_choose_expr(__same_type((s) + 0, const char *), \
+			      (const char *)strnstr(s, t, n),	  \
+			      strnstr(s, t, n)))
+#endif
+
+#endif /* __CONST_LAUNDER */
+
+
 #endif /* _LINUX_STRING_H_ */
diff --git a/lib/string.c b/lib/string.c
index 2c0900a5d51a..89871c6d57cf 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -367,7 +367,7 @@ EXPORT_SYMBOL(strncmp);
  * @s: The string to be searched
  * @c: The character to search for
  */
-char *strchr(const char *s, int c)
+char *(strchr)(const char *s, int c)
 {
 	for (; *s != (char)c; ++s)
 		if (*s == '\0')
@@ -386,7 +386,7 @@ EXPORT_SYMBOL(strchr);
  * Returns pointer to first occurrence of 'c' in s. If c is not found, then
  * return a pointer to the null byte at the end of s.
  */
-char *strchrnul(const char *s, int c)
+char *(strchrnul)(const char *s, int c)
 {
 	while (*s && *s != (char)c)
 		s++;
@@ -401,7 +401,7 @@ EXPORT_SYMBOL(strchrnul);
  * @s: The string to be searched
  * @c: The character to search for
  */
-char *strrchr(const char *s, int c)
+char *(strrchr)(const char *s, int c)
 {
 	const char *last = NULL;
 	do {
@@ -420,7 +420,7 @@ EXPORT_SYMBOL(strrchr);
  * @count: The number of characters to be searched
  * @c: The character to search for
  */
-char *strnchr(const char *s, size_t count, int c)
+char *(strnchr)(const char *s, size_t count, int c)
 {
 	for (; count-- && *s != '\0'; ++s)
 		if (*s == (char)c)
@@ -896,7 +896,7 @@ EXPORT_SYMBOL(memscan);
  * @s1: The string to be searched
  * @s2: The string to search for
  */
-char *strstr(const char *s1, const char *s2)
+char *(strstr)(const char *s1, const char *s2)
 {
 	size_t l1, l2;
 
@@ -922,7 +922,7 @@ EXPORT_SYMBOL(strstr);
  * @s2: The string to search for
  * @len: the maximum number of characters to search
  */
-char *strnstr(const char *s1, const char *s2, size_t len)
+char *(strnstr)(const char *s1, const char *s2, size_t len)
 {
 	size_t l2;
 
-- 
2.16.4

[PATCH 2/4] drivers/base/devtmpfs.c: don't pretend path is const in delete_path

[Rasmus Villemoes]

path is the result of kstrdup, and we repeatedly call strrchr on it,
modifying it through the returned pointer. So there's no reason to
pretend path is const.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/base/devtmpfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index f7768077e817..b93fc862d365 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -252,7 +252,7 @@ static int dev_rmdir(const char *name)
 
 static int delete_path(const char *nodepath)
 {
-	const char *path;
+	char *path;
 	int err = 0;
 
 	path = kstrdup(nodepath, GFP_KERNEL);
-- 
2.16.4

[PATCH 3/4] lib/, include/: avoid const-laundering warnings

[Rasmus Villemoes]

Even though str*() launders the const away for us, we should and do not
modify the buffer through the returned pointer, so might as well declare
it const.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/cpumask.h | 2 +-
 lib/bitmap.c            | 2 +-
 lib/parser.c            | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 147bdec42215..82f4e39b4dc6 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -633,7 +633,7 @@ static inline int cpumask_parselist_user(const char __user *buf, int len,
  */
 static inline int cpumask_parse(const char *buf, struct cpumask *dstp)
 {
-	char *nl = strchr(buf, '\n');
+	const char *nl = strchr(buf, '\n');
 	unsigned int len = nl ? (unsigned int)(nl - buf) : strlen(buf);
 
 	return bitmap_parse(buf, len, cpumask_bits(dstp), nr_cpumask_bits);
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 730969c681cb..de47d7ebbb5f 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -620,7 +620,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 
 int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
 {
-	char *nl  = strchrnul(bp, '\n');
+	const char *nl  = strchrnul(bp, '\n');
 	int len = nl - bp;
 
 	return __bitmap_parselist(bp, len, 0, maskp, nmaskbits);
diff --git a/lib/parser.c b/lib/parser.c
index 3278958b472a..ffb09e9cc8f4 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -25,7 +25,7 @@
  */
 static int match_one(char *s, const char *p, substring_t args[])
 {
-	char *meta;
+	const char *meta;
 	int argc = 0;
 
 	if (!p)
-- 
2.16.4

[PATCH 4/4] kernel/, init/: avoid const-laundering warnings

[Rasmus Villemoes]

Even though str*() launders the const away for us, we should and do not
modify the buffer through the returned pointer, so might as well declare
it const.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 init/do_mounts.c | 2 +-
 kernel/module.c  | 2 +-
 kernel/params.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index 2c71dabe5626..6eea8bfc8f82 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -122,7 +122,7 @@ static dev_t devt_from_partuuid(const char *uuid_str)
 	struct hd_struct *part;
 	int offset = 0;
 	bool clear_root_wait = false;
-	char *slash;
+	const char *slash;
 
 	cmp.uuid = uuid_str;
 
diff --git a/kernel/module.c b/kernel/module.c
index b046a32520d8..8d7ead11a10f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4061,7 +4061,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
 unsigned long module_kallsyms_lookup_name(const char *name)
 {
 	struct module *mod;
-	char *colon;
+	const char *colon;
 	unsigned long ret = 0;
 
 	/* Don't lock: we're in enough trouble already. */
diff --git a/kernel/params.c b/kernel/params.c
index ce89f757e6da..8744aa1e7d8f 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -816,7 +816,7 @@ static void __init param_sysfs_builtin(void)
 	char modname[MODULE_NAME_LEN];
 
 	for (kp = __start___param; kp < __stop___param; kp++) {
-		char *dot;
+		const char *dot;
 
 		if (kp->perm == 0)
 			continue;
-- 
2.16.4

Re: [PATCH 1/4] string: try to find const-laundering bugs

[Joe Perches]

On Wed, 2018-08-22 at 13:00 +0200, Rasmus Villemoes wrote:
> This wraps strchr and friends in macros that ensure the return value has
> type const char* if the passed-in string (which the return value points
> into) also has type const char*. The (s)+0 thing is to force a const
> char[] (e.g. a string literal) to decay to a const char* for the
> __same_type comparison.
[]
> diff --git a/include/linux/string.h b/include/linux/string.h
[]
> +#define strchr(s, c) (						  \
> +	__builtin_choose_expr(__same_type((s) + 0, const char *), \
> +			      (const char *)strchr(s, c),	  \
> +			      strchr(s, c)))
> +#endif
[]
> diff --git a/lib/string.c b/lib/string.c
[]
> @@ -367,7 +367,7 @@ EXPORT_SYMBOL(strncmp);
>   * @s: The string to be searched
>   * @c: The character to search for
>   */
> -char *strchr(const char *s, int c)
> +char *(strchr)(const char *s, int c)

I've tried to use this macro/function wrapping
a few times before, but it seems that it's fairly
unusual in the kernel.  I believe there may not
be any current uses of that style.

A comment explaining the form might be useful.

Re: [PATCH 1/4] string: try to find const-laundering bugs

[Rasmus Villemoes]

On 2018-08-22 13:07, Joe Perches wrote:
> On Wed, 2018-08-22 at 13:00 +0200, Rasmus Villemoes wrote:
>> This wraps strchr and friends in macros that ensure the return value has
>> type const char* if the passed-in string (which the return value points
>> into) also has type const char*. The (s)+0 thing is to force a const
>> char[] (e.g. a string literal) to decay to a const char* for the
>> __same_type comparison.
> []
>> diff --git a/include/linux/string.h b/include/linux/string.h
> []
>> +#define strchr(s, c) (						  \
>> +	__builtin_choose_expr(__same_type((s) + 0, const char *), \
>> +			      (const char *)strchr(s, c),	  \
>> +			      strchr(s, c)))
>> +#endif
> []
>> diff --git a/lib/string.c b/lib/string.c
> []
>> @@ -367,7 +367,7 @@ EXPORT_SYMBOL(strncmp);
>>   * @s: The string to be searched
>>   * @c: The character to search for
>>   */
>> -char *strchr(const char *s, int c)
>> +char *(strchr)(const char *s, int c)
> 
> I've tried to use this macro/function wrapping
> a few times before, but it seems that it's fairly
> unusual in the kernel.  I believe there may not
> be any current uses of that style.
> 
> A comment explaining the form might be useful.
> 

True. I dislike the more conventional #undef strchr approach, because
that would mean any other function in string.c that calls strchr and
happens to be defined after strchr() would not be 'instrumented'. It's
more a principle than a practical thing because, well, none of the
instrumented functions happen to be used by other functions in string.c.

Rasmus