Hi, On Wed, Dec 6, 2023 at 3:38 AM Daniel Thompson <daniel.thomp...@linaro.org> wrote: > > On Tue, Dec 05, 2023 at 01:41:57PM -0800, Doug Anderson wrote: > > Hi, > > > > On Sun, Nov 19, 2023 at 4:10 PM Yuran Pereira <yuran.pere...@hotmail.com> > > 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.pere...@hotmail.com> > > > --- > > > kernel/trace/trace_kdb.c | 14 ++++++-------- > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > > > diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c > > > index 59857a1ee44c..3891f885e4a6 100644 > > > --- a/kernel/trace/trace_kdb.c > > > +++ b/kernel/trace/trace_kdb.c > > > @@ -96,23 +96,21 @@ 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) > > > + if (argc) > > > + if (kstrtoint(argv[1], 0, &skip_entries)) > > > skip_entries = 0; > > > - } > > > > Similar nit about braces as in patch #1. tl;dr is change the above to: > > > > if (argc && kstrtoint(argv[1], 0, &skip_entries)) > > skip_entries = 0; > > Surely that should be: > > if (...) > return KDB_BADINT; > > There seems little point switching to a "safer" API if we just ignore the > errors it provides us.
Ah, sure. I have no objections to that. Note that would have also been possible with the old code, which did still do awkward error checking, so I assumed that it was a conscious decision. ...but I'm definitely happier with the error being reported instead of glossed over. -Doug
