linux-kernel - Re: [PATCH] reboot: fix parsing of reboot cpu number

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Oct 2020 14:20:39 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Matteo Croce <mcroce@...ux.microsoft.com>
Cc:     linux-kernel@...r.kernel.org, Guenter Roeck <linux@...ck-us.net>,
        Arnd Bergmann <arnd@...db.de>, Mike Rapoport <rppt@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Pavel Tatashin <pasha.tatashin@...een.com>,
        Robin Holt <robinmholt@...il.com>,
        Fabian Frederick <fabf@...net.be>, stable@...r.kernel.org
Subject: Re: [PATCH] reboot: fix parsing of reboot cpu number

On Wed 2020-10-14 23:27:46, Matteo Croce wrote:
> From: Matteo Croce <mcroce@...rosoft.com>
> 
> The kernel cmdline reboot= argument allows to specify the CPU used
> for rebooting, with the syntax `s####` among the other flags, e.g.
> 
>   reboot=soft,s4
>   reboot=warm,s31,force
> 
> In the early days the parsing was done with simple_strtoul(), later
> deprecated in favor of the safer kstrtoint() which handles overflow.
> 
> But kstrtoint() returns -EINVAL if there are non-digit characters
> in a string, so if this flag is not the last given, it's silently
> ignored as well as the subsequent ones.
> 
> To fix it, use _parse_integer() which still handles overflow, but
> restores the old behaviour of parsing until a non-digit character
> is found.

Hmm, _parse_integer() is an internal function. And even the comment
says "Don't you dare use this function."

I guess the it is because the base must be hardcoded. And
KSTRTOX_OVERFLOW bit must be handled.

I suggest to go back to simple_strtoul(). It is not longer obsolete.
It still exists because it is needed for exactly this purpose,
see the comment in include/linux/kernel.h

The potentional overflow is not a big deal. The result will be
that the system will reboot on another rCPU than expected. But
it might happen also with any typo.

> While at it, limit the CPU number to num_possible_cpus(),
> because setting it to a value lower than INT_MAX but higher
> than NR_CPUS produces the following error on reboot and shutdown:

Great catch! Please, fix this in a separate patch.

Best Regards,
Petr