LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Yi Yang <yi.y.yang@intel.com>
To: akpm@linux-foundation.org
Cc: linux-kernel@vger.kernel.org, randy.dunlap@oracle.com, gregkh@suse.de
Subject: [PATCH 2.6.24] Add new string functions strict_strto* and convert kernel params to use them, take 3
Date: Fri, 01 Feb 2008 08:02:56 +0800	[thread overview]
Message-ID: <1201824176.3925.13.camel@yangyi-dev.bj.intel.com> (raw)
In-Reply-To: <1201817503.3925.5.camel@yangyi-dev.bj.intel.com>

Andrew, i'm really very very sorry for those mistakes, here is the latest, it
adds documentation for every new function, please use it to replace the patch
in -mm tree you added just now:

add-new-string-functions-strict_strto-and-convert-kernel-params-to-use-them.patch

Thank you, Andrew and Randy.

Currently, for every sysfs node, the callers will be responsible for
implementing store operation, so many many callers are doing duplicate
things to validate input, they have the same mistakes because they are
calling simple_strtol/ul/ll/uul, especially for module params, they are
just numeric, but you can echo such values as 0x1234xxx, 07777888 and
1234aaa, for these cases, module params store operation just ignores
succesive invalid char and converts prefix part to a numeric although
input is acctually invalid.

This patch tries to fix the aforementioned issues and implements strict_strtox
serial functions, kernel/params.c uses them to strictly validate input,
so module params will reject such values as 0x1234xxxx and returns an error:

write error: Invalid argument

Any modules which export numeric sysfs node can use strict_strtox instead of
simple_strtox to reject any invalid input. Please consider to merge to -mm tree
in order to test.

Here are some test results:

Before applying this patch:

[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#


After applying this patch:

[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak
-bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak
-bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak
-bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak
-bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo -n 4096 > /sys/module/e1000/parameters/copybreak
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 include/linux/kernel.h |    4 +
 kernel/params.c        |   20 +++----
 lib/vsprintf.c         |  123 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 10 deletions(-)

--- a/include/linux/kernel.h	2008-01-31 00:41:46.000000000 +0800
+++ b/include/linux/kernel.h	2008-02-01 04:30:49.000000000 +0800
@@ -141,6 +141,10 @@ extern unsigned long simple_strtoul(cons
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
 extern long long simple_strtoll(const char *,char **,unsigned int);
+extern int strict_strtoul(const char *, unsigned int, unsigned long *);
+extern int strict_strtol(const char *, unsigned int, long *);
+extern int strict_strtoull(const char *, unsigned int, unsigned long long *);
+extern int strict_strtoll(const char *, unsigned int, long long *);
 extern int sprintf(char * buf, const char * fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern int vsprintf(char *buf, const char *, va_list)
--- a/lib/vsprintf.c	2008-01-30 08:13:03.000000000 +0800
+++ b/lib/vsprintf.c	2008-02-01 07:36:57.000000000 +0800
@@ -126,6 +126,129 @@ long long simple_strtoll(const char *cp,
 	return simple_strtoull(cp,endp,base);
 }
 
+
+/**
+ * strict_strtoul - convert a string to an unsigned long strictly
+ * @cp: The string to be converted
+ * @base: The number base to use
+ * @res: The converted result value
+ *
+ * strict_strtoul converts a string to an unsigned long only if the
+ * string is really an unsigned long string, any string containing
+ * any invalid char at the tail will be rejected and -EINVAL is returned,
+ * only a newline char at the tail is acceptible because people generally
+ * change a module parameter in the following way:
+ *
+ * 	echo 1024 > /sys/module/e1000/parameters/copybreak
+ *
+ * echo will append a newline to the tail.
+ *
+ * It returns 0 if conversion is successful and *res is set to the converted
+ * value, otherwise it returns -EINVAL and *res is set to 0.
+ *
+ * simple_strtoul just ignores the successive invalid characters and
+ * return the converted value of prefix part of the string.
+ */
+int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
+
+/**
+ * strict_strtol - convert a string to a long strictly
+ * @cp: The string to be converted
+ * @base: The number base to use
+ * @res: The converted result value
+ *
+ * strict_strtol is similiar to strict_strtoul, but it allows the first
+ * character of a string is '-'.
+ *
+ * It returns 0 if conversion is successful and *res is set to the converted
+ * value, otherwise it returns -EINVAL and *res is set to 0.
+ */
+int strict_strtol(const char *cp, unsigned int base, long *res);
+
+/**
+ * strict_strtoull - convert a string to an unsigned long long strictly
+ * @cp: The string to be converted
+ * @base: The number base to use
+ * @res: The converted result value
+ *
+ * strict_strtoull converts a string to an unsigned long long only if the
+ * string is really an unsigned long long string, any string containing
+ * any invalid char at the tail will be rejected and -EINVAL is returned,
+ * only a newline char at the tail is acceptible because people generally
+ * change a module parameter in the following way:
+ *
+ * 	echo 1024 > /sys/module/e1000/parameters/copybreak
+ *
+ * echo will append a newline to the tail of the string.
+ *
+ * It returns 0 if conversion is successful and *res is set to the converted
+ * value, otherwise it returns -EINVAL and *res is set to 0.
+ *
+ * simple_strtoull just ignores the successive invalid characters and
+ * return the converted value of prefix part of the string.
+ */
+int strict_strtoull(const char *cp, unsigned int base, unsigned long long *res);
+
+/**
+ * strict_strtoll - convert a string to a long long strictly
+ * @cp: The string to be converted
+ * @base: The number base to use
+ * @res: The converted result value
+ *
+ * strict_strtoll is similiar to strict_strtoull, but it allows the first
+ * character of a string is '-'.
+ *
+ * It returns 0 if conversion is successful and *res is set to the converted
+ * value, otherwise it returns -EINVAL and *res is set to 0.
+ */
+int strict_strtoll(const char *cp, unsigned int base, long long *res);
+
+#define define_strict_strtoux(type, valtype)				\
+int strict_strtou##type(const char *cp, unsigned int base, valtype *res)\
+{									\
+	char *tail;							\
+	valtype val;							\
+	size_t len;							\
+									\
+	*res = 0;							\
+	len = strlen(cp);						\
+	if (len == 0)							\
+		return -EINVAL;						\
+									\
+	val = simple_strtoul(cp, &tail, base);				\
+	if ((*tail == '\0') ||						\
+		(len == (size_t)(tail - cp) + 1) && (*tail == '\n')) {	\
+		*res = val;						\
+		return 0;						\
+	}								\
+									\
+	return -EINVAL;							\
+}									\
+
+#define define_strict_strtox(type, valtype)				\
+int strict_strto##type(const char *cp, unsigned int base, valtype *res)	\
+{									\
+	int ret;							\
+	if (*cp == '-') {						\
+		ret = strict_strtou##type(cp+1, base, res);		\
+		if (ret != 0)						\
+			*res = -(*res);					\
+	} else								\
+		ret = strict_strtou##type(cp+1, base, res);		\
+									\
+	return ret;							\
+}									\
+
+define_strict_strtoux(l, unsigned long)
+define_strict_strtox(l, long)
+define_strict_strtoux(ll, unsigned long long)
+define_strict_strtox(ll, long long)
+
+EXPORT_SYMBOL(strict_strtoul);
+EXPORT_SYMBOL(strict_strtol);
+EXPORT_SYMBOL(strict_strtoll);
+EXPORT_SYMBOL(strict_strtoull);
+
 static int skip_atoi(const char **s)
 {
 	int i=0;
--- a/kernel/params.c	2008-01-31 00:44:44.000000000 +0800
+++ b/kernel/params.c	2008-02-01 04:32:12.000000000 +0800
@@ -180,12 +180,12 @@ int parse_args(const char *name,
 #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)      	\
 	int param_set_##name(const char *val, struct kernel_param *kp)	\
 	{								\
-		char *endp;						\
 		tmptype l;						\
+		int ret;						\
 									\
 		if (!val) return -EINVAL;				\
-		l = strtolfn(val, &endp, 0);				\
-		if (endp == val || ((type)l != l))			\
+		ret = strtolfn(val, 0, &l);				\
+		if (ret == -EINVAL || ((type)l != l))			\
 			return -EINVAL;					\
 		*((type *)kp->arg) = l;					\
 		return 0;						\
@@ -195,13 +195,13 @@ int parse_args(const char *name,
 		return sprintf(buffer, format, *((type *)kp->arg));	\
 	}
 
-STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(short, short, "%hi", long, simple_strtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(int, int, "%i", long, simple_strtol);
-STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(long, long, "%li", long, simple_strtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, simple_strtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul);
+STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol);
+STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul);
+STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol);
+STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, strict_strtoul);
+STANDARD_PARAM_DEF(long, long, "%li", long, strict_strtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {



  reply	other threads:[~2008-02-01  9:16 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-04 10:10 [linux-pm][PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device Yi Yang
2008-01-04 11:48 ` Pavel Machek
2008-01-04 16:09   ` David Brownell
2008-01-04 16:38     ` Alan Stern
2008-01-04 16:52       ` Olivier Galibert
2008-01-04 17:20         ` Oliver Neukum
2008-01-07  2:00           ` Yi Yang
2008-01-07  1:55         ` Yi Yang
2008-01-07  3:49           ` David Brownell
2008-01-07  1:52       ` Yi Yang
2008-01-07  1:37     ` Yi Yang
2008-01-07  1:57       ` Rafael J. Wysocki
2008-01-07  2:05         ` Yi Yang
2008-01-07 16:46           ` Rafael J. Wysocki
2008-01-07  1:21   ` Yi Yang
2008-01-04 16:31 ` David Brownell
2008-01-07  1:51   ` Yi Yang
2008-01-22 23:05 ` [PATCH 2.6.24-rc8] cpufreq: fix obvious condition statement error Yi Yang
2008-01-28 23:14   ` [PATCH 2.6.24] x86: add sysfs interface for cpuid module Yi Yang
2008-01-29  8:44     ` Sam Ravnborg
2008-01-29 22:22       ` Yi Yang
2008-01-30  7:48         ` Sam Ravnborg
2008-01-29 15:51     ` H. Peter Anvin
2008-01-29 17:52       ` Yi Yang
2008-01-30  6:09         ` H. Peter Anvin
2008-01-29 21:56       ` Yi Yang
2008-01-30  7:17         ` H. Peter Anvin
2008-01-29 22:13           ` Yi Yang
2008-01-30  7:33             ` H. Peter Anvin
2008-01-29 23:41     ` [RFC PATCH 2.6.24] x86: add sysfs interface for cpuid module, try 2 Yi Yang
2008-01-30 17:35       ` H. Peter Anvin
2008-01-31  1:18       ` [PATCH 2.6.24] Add new string functions real_strtoul and change kernel params to use them Yi Yang
2008-01-31 17:03         ` Randy Dunlap
2008-01-31 16:30           ` Yi Yang
2008-02-01  2:47             ` Randy Dunlap
2008-01-31 22:11         ` [PATCH 2.6.24] Add new string functions strict_strto* and convert kernel params to use them, take 2 Yi Yang
2008-02-01  0:02           ` Yi Yang [this message]
2008-02-01  7:36           ` Andrew Morton
2008-02-01  7:39           ` Andrew Morton
2008-02-01  7:40           ` Andrew Morton
2008-02-01 16:31     ` [PATCH 2.6.24] x86: add sysfs interface for cpuid module dean gaudet
2008-02-01 17:56       ` H. Peter Anvin
2008-02-01 17:58         ` H. Peter Anvin
2008-02-14 23:44   ` [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance Yi Yang
2008-02-14 23:48   ` Yi Yang
2008-02-15 15:52     ` [linux-pm] " Alan Stern
2008-02-15 18:24       ` Greg KH
2008-02-15 21:01     ` Greg KH
2008-02-25  0:46     ` [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow Yi Yang
2008-02-25 10:15       ` Ingo Molnar
2008-02-25  1:10         ` Yi Yang
2008-03-26  4:46       ` Len Brown

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=1201824176.3925.13.camel@yangyi-dev.bj.intel.com \
    --to=yi.y.yang@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=gregkh@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=randy.dunlap@oracle.com \
    --subject='Re: [PATCH 2.6.24] Add new string functions strict_strto* and convert kernel params to use them, take 3' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).