| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <556C25CE.4020006@linaro.org>
Date: Mon, 01 Jun 2015 10:28:46 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Filip Ayazi <filipayazi@...il.com>, jason.wessel@...driver.com
CC: adobriyan@...il.com, linux-kernel@...r.kernel.org,
kgdb-bugreport@...ts.sourceforge.net
Subject: Re: [PATCH v2] kdb: use kstrto* instead of simple_strto
On 31/05/15 18:59, Filip Ayazi wrote:
> Replace simple_strto with appropriate kstrto functions as recommended
> by checkpatch, change pid, sig types to unsigned int, int respectively.
A changelog describing the changes are in v2 would be nice here.
> Signed-off-by: Filip Ayazi <filipayazi@...il.com>
> ---
> kernel/debug/kdb/kdb_main.c | 56 +++++++++++++++------------------------------
> 1 file changed, 19 insertions(+), 37 deletions(-)
I can't find any bugs introduced by this patch although I do have some
cosmetic nitpicks below. You can add my reviewed-by to the next release:
Reviewed-by: Daniel Thompson <daniel.thompson@...aro.org>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 4121345..6458330 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -296,7 +296,8 @@ static int kdbgetulenv(const char *match, unsigned long *value)
> if (strlen(ep) == 0)
> return KDB_NOENVVALUE;
>
> - *value = simple_strtoul(ep, NULL, 0);
> + if (kstrtoul(ep, 0, value) != 0)
^^^^^
Checking for != 0 is what if does anyway. A generally clearer code
pattern for kernel code that returns 0 on success is:
err = kstrtoul(...);
if (err)
handle_error();
> + return KDB_BADINT;
>
> return 0;
> }
> @@ -334,20 +335,15 @@ int kdbgetintenv(const char *match, int *value)
> */
> int kdbgetularg(const char *arg, unsigned long *value)
> {
> - char *endp;
> unsigned long val;
>
> - val = simple_strtoul(arg, &endp, 0);
> -
> - if (endp == arg) {
> - /*
> - * Also try base 16, for us folks too lazy to type the
> - * leading 0x...
> - */
> - val = simple_strtoul(arg, &endp, 16);
> - if (endp == arg)
> + /*
> + * Also try base 16, for us folks too lazy to type the
> + * leading 0x...
> + */
> + if (kstrtoul(arg, 0, &val) != 0)
> + if (kstrtoul(arg, 16, &val) != 0)
> return KDB_BADINT;
Whether or not it is good taste to introduce 'err' here is debatable
since it makes the code flow pretty verbose. However it would still be
good to get rid of the '!= 0'.
> - }
>
> *value = val;
kstrtoul() does not write to its argument unless it is successful. That
means we no longer need a temporary variable here; just pass value into
kstrtoul().
> @@ -356,17 +352,11 @@ int kdbgetularg(const char *arg, unsigned long *value)
>
> int kdbgetu64arg(const char *arg, u64 *value)
> {
> - char *endp;
> u64 val;
>
> - val = simple_strtoull(arg, &endp, 0);
> -
> - if (endp == arg) {
> -
> - val = simple_strtoull(arg, &endp, 16);
> - if (endp == arg)
> + if (kstrtoull(arg, 0, &val) != 0)
> + if (kstrtoull(arg, 16, &val) != 0)
> return KDB_BADINT;
+1
> - }
>
> *value = val;
>
> @@ -402,10 +392,9 @@ int kdb_set(int argc, const char **argv)
> */
> if (strcmp(argv[1], "KDBDEBUG") == 0) {
> unsigned int debugflags;
> - char *cp;
>
> - debugflags = simple_strtoul(argv[2], &cp, 0);
> - if (cp == argv[2] || debugflags & ~KDB_DEBUG_FLAG_MASK) {
> + if (kstrtouint(argv[2], 0, &debugflags) != 0
> + || debugflags & ~KDB_DEBUG_FLAG_MASK) {
+1
> kdb_printf("kdb: illegal debug flags '%s'\n",
> argv[2]);
> return 0;
> @@ -1588,10 +1577,8 @@ static int kdb_md(int argc, const char **argv)
> if (!argv[0][3])
> valid = 1;
> else if (argv[0][3] == 'c' && argv[0][4]) {
> - char *p;
> - repeat = simple_strtoul(argv[0] + 4, &p, 10);
> + valid = !kstrtoint(argv[0]+4, 10, &repeat);
> mdcount = ((repeat * bytesperword) + 15) / 16;
> - valid = !*p;
> }
> last_repeat = repeat;
Interesting! Your changes mean we no longer store the, known invalid,
return value from simple_strtoul into last_repeat. Since last_repeat is
a static variable and can influence future commands that's a *good* change.
> } else if (strcmp(argv[0], "md") == 0)
> @@ -2091,13 +2078,10 @@ static int kdb_dmesg(int argc, const char **argv)
> if (argc > 2)
> return KDB_ARGCOUNT;
> if (argc) {
> - char *cp;
> - lines = simple_strtol(argv[1], &cp, 0);
> - if (*cp)
> + if (kstrtoint(argv[1], 0, &lines) != 0)
> lines = 0;
> if (argc > 1) {
> - adjust = simple_strtoul(argv[2], &cp, 0);
> - if (*cp || adjust < 0)
> + if (kstrtoint(argv[2], 0, &adjust) != 0 || adjust < 0)
> adjust = 0;
+2
> }
> }
> @@ -2436,16 +2420,15 @@ static int kdb_help(int argc, const char **argv)
> */
> static int kdb_kill(int argc, const char **argv)
> {
> - long sig, pid;
> - char *endp;
> + unsigned int pid;
Why not use pid_t/kstrtoint() here? The code already has a check to cope
with the user passing a negative value.
> + int sig;
> struct task_struct *p;
> struct siginfo info;
>
> if (argc != 2)
> return KDB_ARGCOUNT;
>
> - sig = simple_strtol(argv[1], &endp, 0);
> - if (*endp)
> + if (kstrtoint(argv[1], 0, &sig) != 0)
> return KDB_BADINT;
> if (sig >= 0) {
> kdb_printf("Invalid signal parameter.<-signal>\n");
> @@ -2453,8 +2436,7 @@ static int kdb_kill(int argc, const char **argv)
> }
> sig = -sig;
>
> - pid = simple_strtol(argv[2], &endp, 0);
> - if (*endp)
> + if (kstrtouint(argv[2], 0, &pid) != 0)
> return KDB_BADINT;
> if (pid <= 0) {
> kdb_printf("Process ID must be large than 0.\n");
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists