| Message ID | 20241021211724.GC835676@lichtman.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Replace the use of simple_strtol/ul functions with kstrto | expand |
On Mon, 21 Oct 2024 21:17:24 +0000 Nir Lichtman <nir@lichtman.org> wrote: > The function simple_strtoul performs no error checking in scenarios > where the input value overflows the intended output variable. > This results in this function successfully returning, even when the > output does not match the input string (aka the function returns > successfully even when the result is wrong). > > Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(), > simple_strtoul(), and simple_strtoull() functions explicitly ignore > overflows, which may lead to unexpected results in callers." > Hence, the use of those functions is discouraged. > > This patch replaces all uses of the simple_strtoul with the safer > alternatives kstrtoint and kstrtol. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull > Looks good to me. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you, > Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com> > Signed-off-by: Nir Lichtman <nir@lichtman.org> > --- > kernel/trace/trace_kdb.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c > index 59857a1ee44c..eadda6e05526 100644 > --- a/kernel/trace/trace_kdb.c > +++ b/kernel/trace/trace_kdb.c > @@ -96,23 +96,20 @@ static int kdb_ftdump(int argc, const char **argv) > { > int skip_entries = 0; > long cpu_file; > - char *cp; > + int err; > int cnt; > int cpu; > > if (argc > 2) > return KDB_ARGCOUNT; > > - if (argc) { > - skip_entries = simple_strtol(argv[1], &cp, 0); > - if (*cp) > - skip_entries = 0; > - } > + if (argc && kstrtoint(argv[1], 0, &skip_entries)) > + return KDB_BADINT; > > if (argc == 2) { > - cpu_file = simple_strtol(argv[2], &cp, 0); > - if (*cp || cpu_file >= NR_CPUS || cpu_file < 0 || > - !cpu_online(cpu_file)) > + err = kstrtol(argv[2], 0, &cpu_file); > + if (err || cpu_file >= NR_CPUS || cpu_file < 0 || > + !cpu_online(cpu_file)) > return KDB_BADINT; > } else { > cpu_file = RING_BUFFER_ALL_CPUS; > -- > 2.39.2
Hi, On Mon, Oct 21, 2024 at 2:17 PM Nir Lichtman <nir@lichtman.org> wrote: > > The function simple_strtoul performs no error checking in scenarios > where the input value overflows the intended output variable. > This results in this function successfully returning, even when the > output does not match the input string (aka the function returns > successfully even when the result is wrong). > > Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(), > simple_strtoul(), and simple_strtoull() functions explicitly ignore > overflows, which may lead to unexpected results in callers." > Hence, the use of those functions is discouraged. > > This patch replaces all uses of the simple_strtoul with the safer > alternatives kstrtoint and kstrtol. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull > > Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com> > Signed-off-by: Nir Lichtman <nir@lichtman.org> I have the same comments as patch #1 about authorship and noting what you changed. > --- > kernel/trace/trace_kdb.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c > index 59857a1ee44c..eadda6e05526 100644 > --- a/kernel/trace/trace_kdb.c > +++ b/kernel/trace/trace_kdb.c > @@ -96,23 +96,20 @@ static int kdb_ftdump(int argc, const char **argv) > { > int skip_entries = 0; > long cpu_file; > - char *cp; > + int err; > int cnt; > int cpu; > > if (argc > 2) > return KDB_ARGCOUNT; > > - if (argc) { > - skip_entries = simple_strtol(argv[1], &cp, 0); > - if (*cp) > - skip_entries = 0; > - } > + if (argc && kstrtoint(argv[1], 0, &skip_entries)) > + return KDB_BADINT; > > if (argc == 2) { > - cpu_file = simple_strtol(argv[2], &cp, 0); > - if (*cp || cpu_file >= NR_CPUS || cpu_file < 0 || > - !cpu_online(cpu_file)) > + err = kstrtol(argv[2], 0, &cpu_file); > + if (err || cpu_file >= NR_CPUS || cpu_file < 0 || > + !cpu_online(cpu_file)) FWIW, this is still changing the indentation of the second line in a way that makes it worse list like v1 did [1]. :-P The "!" in "!cpu_" should line up right under the "e" in "err". That's a pretty tiny nit, so I'm OK with: Reviewed-by: Douglas Anderson <dianders@chromium.org> When sending v3 you'd want to keep Masami's Acked-by tag and my Reviewed-by tag. They should go just above your Signed-off-by tag.
diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c index 59857a1ee44c..eadda6e05526 100644 --- a/kernel/trace/trace_kdb.c +++ b/kernel/trace/trace_kdb.c @@ -96,23 +96,20 @@ static int kdb_ftdump(int argc, const char **argv) { int skip_entries = 0; long cpu_file; - char *cp; + int err; int cnt; int cpu; if (argc > 2) return KDB_ARGCOUNT; - if (argc) { - skip_entries = simple_strtol(argv[1], &cp, 0); - if (*cp) - skip_entries = 0; - } + if (argc && kstrtoint(argv[1], 0, &skip_entries)) + return KDB_BADINT; if (argc == 2) { - cpu_file = simple_strtol(argv[2], &cp, 0); - if (*cp || cpu_file >= NR_CPUS || cpu_file < 0 || - !cpu_online(cpu_file)) + err = kstrtol(argv[2], 0, &cpu_file); + if (err || cpu_file >= NR_CPUS || cpu_file < 0 || + !cpu_online(cpu_file)) return KDB_BADINT; } else { cpu_file = RING_BUFFER_ALL_CPUS;