* [PATCH v2 0/2] fix parsing of reboot= cmdline
@ 2020-10-27 13:35 Matteo Croce
2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Matteo Croce @ 2020-10-27 13:35 UTC (permalink / raw)
To: linux-kernel
Cc: Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport,
Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable
From: Matteo Croce <mcroce@microsoft.com>
The parsing of the reboot= cmdline has two major errors:
- a missing bound check can crash the system on reboot
- parsing of the cpu number only works if specified last
Fix both, along with a small code refactor.
v1->v2:
As Petr suggested, don't force base 10 in simple_strtoul(),
so hex values are accepted as well.
Matteo Croce (2):
reboot: fix overflow parsing reboot cpu number
reboot: fix parsing of reboot cpu number
kernel/reboot.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number 2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce @ 2020-10-27 13:35 ` Matteo Croce 2020-10-27 13:42 ` Greg KH 2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce 2020-10-27 13:42 ` [PATCH v2 0/2] fix parsing of reboot= cmdline Greg KH 2 siblings, 1 reply; 15+ messages in thread From: Matteo Croce @ 2020-10-27 13:35 UTC (permalink / raw) To: linux-kernel Cc: Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable From: Matteo Croce <mcroce@microsoft.com> 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: BUG: unable to handle page fault for address: ffffffff90ab1bb0 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 1c09067 P4D 1c09067 PUD 1c0a063 PMD 0 Oops: 0000 [#1] SMP CPU: 1 PID: 1 Comm: systemd-shutdow Not tainted 5.9.0-rc8-kvm #110 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014 RIP: 0010:migrate_to_reboot_cpu+0xe/0x60 Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c RSP: 0018:ffffc90000013e08 EFLAGS: 00010246 RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000 RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0 RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000 FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: __do_sys_reboot.cold+0x34/0x5b ? vfs_writev+0x92/0xc0 ? do_writev+0x52/0xd0 do_syscall_64+0x2d/0x40 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f32bfaaecd3 Code: 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 89 fa be 69 19 12 28 bf ad de e1 fe b8 a9 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 89 81 0c 00 f7 d8 RSP: 002b:00007fff6265fb58 EFLAGS: 00000202 ORIG_RAX: 00000000000000a9 RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f32bfaaecd3 RDX: 0000000001234567 RSI: 0000000028121969 RDI: 00000000fee1dead RBP: 0000000000000000 R08: 0000000000008020 R09: 00007fff6265ef60 R10: 00007f32bedf8830 R11: 0000000000000202 R12: 0000000000000000 R13: 0000557bba2c51c0 R14: 0000000000000000 R15: 00007fff6265fbc8 CR2: ffffffff90ab1bb0 ---[ end trace b813e80157136563 ]--- RIP: 0010:migrate_to_reboot_cpu+0xe/0x60 Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c RSP: 0018:ffffc90000013e08 EFLAGS: 00010246 RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000 RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0 RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000 FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 Kernel Offset: disabled ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]--- Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel") Signed-off-by: Matteo Croce <mcroce@microsoft.com> --- kernel/reboot.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/kernel/reboot.c b/kernel/reboot.c index e7b78d5ae1ab..c4e7965c39b9 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -558,11 +558,19 @@ static int __init reboot_setup(char *str) rc = kstrtoint(str+1, 0, &reboot_cpu); if (rc) return rc; + if (reboot_cpu >= num_possible_cpus()) { + reboot_cpu = 0; + return -ERANGE; + } } else if (str[1] == 'm' && str[2] == 'p' && isdigit(*(str+3))) { rc = kstrtoint(str+3, 0, &reboot_cpu); if (rc) return rc; + if (reboot_cpu >= num_possible_cpus()) { + reboot_cpu = 0; + return -ERANGE; + } } else *mode = REBOOT_SOFT; break; -- 2.28.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number 2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce @ 2020-10-27 13:42 ` Greg KH 2020-10-30 14:13 ` Petr Mladek 0 siblings, 1 reply; 15+ messages in thread From: Greg KH @ 2020-10-27 13:42 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable On Tue, Oct 27, 2020 at 02:35:44PM +0100, Matteo Croce wrote: > From: Matteo Croce <mcroce@microsoft.com> > > 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: > > BUG: unable to handle page fault for address: ffffffff90ab1bb0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 1c09067 P4D 1c09067 PUD 1c0a063 PMD 0 > Oops: 0000 [#1] SMP > CPU: 1 PID: 1 Comm: systemd-shutdow Not tainted 5.9.0-rc8-kvm #110 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014 > RIP: 0010:migrate_to_reboot_cpu+0xe/0x60 > Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c > RSP: 0018:ffffc90000013e08 EFLAGS: 00010246 > RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000 > RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0 > RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348 > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000 > FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > __do_sys_reboot.cold+0x34/0x5b > ? vfs_writev+0x92/0xc0 > ? do_writev+0x52/0xd0 > do_syscall_64+0x2d/0x40 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x7f32bfaaecd3 > Code: 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 89 fa be 69 19 12 28 bf ad de e1 fe b8 a9 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 89 81 0c 00 f7 d8 > RSP: 002b:00007fff6265fb58 EFLAGS: 00000202 ORIG_RAX: 00000000000000a9 > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f32bfaaecd3 > RDX: 0000000001234567 RSI: 0000000028121969 RDI: 00000000fee1dead > RBP: 0000000000000000 R08: 0000000000008020 R09: 00007fff6265ef60 > R10: 00007f32bedf8830 R11: 0000000000000202 R12: 0000000000000000 > R13: 0000557bba2c51c0 R14: 0000000000000000 R15: 00007fff6265fbc8 > CR2: ffffffff90ab1bb0 > ---[ end trace b813e80157136563 ]--- > RIP: 0010:migrate_to_reboot_cpu+0xe/0x60 > Code: ea ea 00 48 89 fa 48 c7 c7 30 57 f1 81 e9 fa ef ff ff 66 2e 0f 1f 84 00 00 00 00 00 53 8b 1d d5 ea ea 00 e8 14 33 fe ff 89 da <48> 0f a3 15 ea fc bd 00 48 89 d0 73 29 89 c2 c1 e8 06 65 48 8b 3c > RSP: 0018:ffffc90000013e08 EFLAGS: 00010246 > RAX: ffff88801f0a0000 RBX: 0000000077359400 RCX: 0000000000000000 > RDX: 0000000077359400 RSI: 0000000000000002 RDI: ffffffff81c199e0 > RBP: ffffffff81c1e3c0 R08: ffff88801f41f000 R09: ffffffff81c1e348 > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > R13: 00007f32bedf8830 R14: 00000000fee1dead R15: 0000000000000000 > FS: 00007f32bedf8980(0000) GS:ffff88801f480000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffffffff90ab1bb0 CR3: 000000001d057000 CR4: 00000000000006a0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 > Kernel Offset: disabled > ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]--- > > Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel") > Signed-off-by: Matteo Croce <mcroce@microsoft.com> > --- > kernel/reboot.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number 2020-10-27 13:42 ` Greg KH @ 2020-10-30 14:13 ` Petr Mladek 2020-10-30 14:18 ` Matteo Croce 0 siblings, 1 reply; 15+ messages in thread From: Petr Mladek @ 2020-10-30 14:13 UTC (permalink / raw) To: Greg KH Cc: Matteo Croce, linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable On Tue 2020-10-27 14:42:43, Greg KH wrote: > On Tue, Oct 27, 2020 at 02:35:44PM +0100, Matteo Croce wrote: > > From: Matteo Croce <mcroce@microsoft.com> > > > > 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: > > > > Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel") > > Signed-off-by: Matteo Croce <mcroce@microsoft.com> Reviewed-by: Petr Mladek <pmladek@suse.com> > <formletter> > > This is not the correct way to submit patches for inclusion in the > stable kernel tree. Please read: > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > for how to do this properly. > > </formletter> The best way is to add the following line before Signed-off-by line: Cc: stable@vger.kernel.org Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number 2020-10-30 14:13 ` Petr Mladek @ 2020-10-30 14:18 ` Matteo Croce 0 siblings, 0 replies; 15+ messages in thread From: Matteo Croce @ 2020-10-30 14:18 UTC (permalink / raw) To: Petr Mladek Cc: Greg KH, linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick On Fri, Oct 30, 2020 at 3:13 PM Petr Mladek <pmladek@suse.com> wrote: > > On Tue 2020-10-27 14:42:43, Greg KH wrote: > > On Tue, Oct 27, 2020 at 02:35:44PM +0100, Matteo Croce wrote: > > > From: Matteo Croce <mcroce@microsoft.com> > > > > > > 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: > > > > > > Fixes: 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel") > > > Signed-off-by: Matteo Croce <mcroce@microsoft.com> > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > > <formletter> > > > > This is not the correct way to submit patches for inclusion in the > > stable kernel tree. Please read: > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > for how to do this properly. > > > > </formletter> > > The best way is to add the following line before Signed-off-by line: > > Cc: stable@vger.kernel.org > I see, sorry for the noise. Should I resend, or I eventually write to stable@vger.kernel.org after the merge? -- per aspera ad upstream ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce 2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce @ 2020-10-27 13:35 ` Matteo Croce 2020-10-27 13:42 ` Greg KH 2020-10-30 14:30 ` Petr Mladek 2020-10-27 13:42 ` [PATCH v2 0/2] fix parsing of reboot= cmdline Greg KH 2 siblings, 2 replies; 15+ messages in thread From: Matteo Croce @ 2020-10-27 13:35 UTC (permalink / raw) To: linux-kernel Cc: Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer deprecated, and restore the old behaviour. While at it, merge two identical code blocks into one. Fixes: 616feab75397 ("kernel/reboot.c: convert simple_strtoul to kstrtoint") Signed-off-by: Matteo Croce <mcroce@microsoft.com> --- kernel/reboot.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/kernel/reboot.c b/kernel/reboot.c index c4e7965c39b9..a09c5937c0b6 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) case 's': { - int rc; - - if (isdigit(*(str+1))) { - rc = kstrtoint(str+1, 0, &reboot_cpu); - if (rc) - return rc; - if (reboot_cpu >= num_possible_cpus()) { - reboot_cpu = 0; - return -ERANGE; - } - } else if (str[1] == 'm' && str[2] == 'p' && - isdigit(*(str+3))) { - rc = kstrtoint(str+3, 0, &reboot_cpu); - if (rc) - return rc; - if (reboot_cpu >= num_possible_cpus()) { - reboot_cpu = 0; + int cpu; + + /* + * reboot_cpu is s[mp]#### with #### being the processor + * to be used for rebooting. Skip 's' or 'smp' prefix. + */ + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; + + if (isdigit(str[0])) { + cpu = simple_strtoul(str, NULL, 0); + if (cpu >= num_possible_cpus()) return -ERANGE; - } + reboot_cpu = cpu; } else *mode = REBOOT_SOFT; break; -- 2.28.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce @ 2020-10-27 13:42 ` Greg KH 2020-10-30 14:30 ` Petr Mladek 1 sibling, 0 replies; 15+ messages in thread From: Greg KH @ 2020-10-27 13:42 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable On Tue, Oct 27, 2020 at 02:35:45PM +0100, Matteo Croce wrote: > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > deprecated, and restore the old behaviour. > > While at it, merge two identical code blocks into one. > > Fixes: 616feab75397 ("kernel/reboot.c: convert simple_strtoul to kstrtoint") > Signed-off-by: Matteo Croce <mcroce@microsoft.com> > --- > kernel/reboot.c | 30 ++++++++++++------------------ > 1 file changed, 12 insertions(+), 18 deletions(-) > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce 2020-10-27 13:42 ` Greg KH @ 2020-10-30 14:30 ` Petr Mladek 2020-11-01 1:57 ` Matteo Croce 1 sibling, 1 reply; 15+ messages in thread From: Petr Mladek @ 2020-10-30 14:30 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > deprecated, and restore the old behaviour. > > While at it, merge two identical code blocks into one. > --- a/kernel/reboot.c > +++ b/kernel/reboot.c > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > case 's': > { > - int rc; > - > - if (isdigit(*(str+1))) { > - rc = kstrtoint(str+1, 0, &reboot_cpu); > - if (rc) > - return rc; > - if (reboot_cpu >= num_possible_cpus()) { > - reboot_cpu = 0; > - return -ERANGE; > - } > - } else if (str[1] == 'm' && str[2] == 'p' && > - isdigit(*(str+3))) { > - rc = kstrtoint(str+3, 0, &reboot_cpu); > - if (rc) > - return rc; > - if (reboot_cpu >= num_possible_cpus()) { > - reboot_cpu = 0; ^^^^^^ > + int cpu; > + > + /* > + * reboot_cpu is s[mp]#### with #### being the processor > + * to be used for rebooting. Skip 's' or 'smp' prefix. > + */ > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > + > + if (isdigit(str[0])) { > + cpu = simple_strtoul(str, NULL, 0); > + if (cpu >= num_possible_cpus()) > return -ERANGE; > - } > + reboot_cpu = cpu; The original value stays when the new one is out of range. It is small functional change that should get mentioned in the commit message or better fixed separately. Hmm, I suggest to split this into 3 patches and switch the order: + 1st patch should simply revert the commit 616feab75397 ("kernel/reboot.c: convert simple_strtoul to kstrtoint"). + 2nd patch should merge the two branches without any functional change. + 3rd patch should add the check for num_possible_cpus() and update the value only when it is valid. I am sorry that I did not suggested this when reviewed v1. I have missed this functional change at that time. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-10-30 14:30 ` Petr Mladek @ 2020-11-01 1:57 ` Matteo Croce 2020-11-02 11:01 ` Petr Mladek 0 siblings, 1 reply; 15+ messages in thread From: Matteo Croce @ 2020-11-01 1:57 UTC (permalink / raw) To: Petr Mladek Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote: > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > > deprecated, and restore the old behaviour. > > > > While at it, merge two identical code blocks into one. > > > --- a/kernel/reboot.c > > +++ b/kernel/reboot.c > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > > > case 's': > > { > > - int rc; > > - > > - if (isdigit(*(str+1))) { > > - rc = kstrtoint(str+1, 0, &reboot_cpu); > > - if (rc) > > - return rc; > > - if (reboot_cpu >= num_possible_cpus()) { > > - reboot_cpu = 0; > > - return -ERANGE; > > - } > > - } else if (str[1] == 'm' && str[2] == 'p' && > > - isdigit(*(str+3))) { > > - rc = kstrtoint(str+3, 0, &reboot_cpu); > > - if (rc) > > - return rc; > > - if (reboot_cpu >= num_possible_cpus()) { > > - reboot_cpu = 0; > > ^^^^^^ > > > + int cpu; > > + > > + /* > > + * reboot_cpu is s[mp]#### with #### being the processor > > + * to be used for rebooting. Skip 's' or 'smp' prefix. > > + */ > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > > + > > + if (isdigit(str[0])) { > > + cpu = simple_strtoul(str, NULL, 0); > > + if (cpu >= num_possible_cpus()) > > return -ERANGE; > > - } > > + reboot_cpu = cpu; > > The original value stays when the new one is out of range. It is > small functional change that should get mentioned in the commit > message or better fixed separately. > Hi, I didn't understand, the original value is 0 since reboot_cpu is global. reboot_setup() is only called once at boot to parse the cmdline, indeed it's __init. I don't know any way to call it more than once (exept passing multiple reboot= arguments) > Hmm, I suggest to split this into 3 patches and switch the order: > > + 1st patch should simply revert the commit 616feab75397 > ("kernel/reboot.c: convert simple_strtoul to kstrtoint"). > > + 2nd patch should merge the two branches without any > functional change. > > + 3rd patch should add the check for num_possible_cpus() > and update the value only when it is valid. > > I am sorry that I did not suggested this when reviewed v1. > I have missed this functional change at that time. > Np :) Bye, -- per aspera ad upstream ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-11-01 1:57 ` Matteo Croce @ 2020-11-02 11:01 ` Petr Mladek 2020-11-03 11:43 ` Matteo Croce 0 siblings, 1 reply; 15+ messages in thread From: Petr Mladek @ 2020-11-02 11:01 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt On Sun 2020-11-01 02:57:40, Matteo Croce wrote: > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote: > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > > > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > > > deprecated, and restore the old behaviour. > > > > > > While at it, merge two identical code blocks into one. > > > > > --- a/kernel/reboot.c > > > +++ b/kernel/reboot.c > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > > > > > case 's': > > > { > > > - int rc; > > > - > > > - if (isdigit(*(str+1))) { > > > - rc = kstrtoint(str+1, 0, &reboot_cpu); > > > - if (rc) > > > - return rc; > > > - if (reboot_cpu >= num_possible_cpus()) { > > > - reboot_cpu = 0; > > > - return -ERANGE; > > > - } > > > - } else if (str[1] == 'm' && str[2] == 'p' && > > > - isdigit(*(str+3))) { > > > - rc = kstrtoint(str+3, 0, &reboot_cpu); > > > - if (rc) > > > - return rc; > > > - if (reboot_cpu >= num_possible_cpus()) { > > > - reboot_cpu = 0; > > > > ^^^^^^ > > > > > + int cpu; > > > + > > > + /* > > > + * reboot_cpu is s[mp]#### with #### being the processor > > > + * to be used for rebooting. Skip 's' or 'smp' prefix. > > > + */ > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > > > + > > > + if (isdigit(str[0])) { > > > + cpu = simple_strtoul(str, NULL, 0); > > > + if (cpu >= num_possible_cpus()) > > > return -ERANGE; > > > - } > > > + reboot_cpu = cpu; > > > > The original value stays when the new one is out of range. It is > > small functional change that should get mentioned in the commit > > message or better fixed separately. Ah, I see. From some reason, I assumed that it was defined as module_param() or core_param(). Then it would be possible to modify it later via /sys. I am sorry for the noise. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-11-02 11:01 ` Petr Mladek @ 2020-11-03 11:43 ` Matteo Croce 2020-11-03 14:25 ` Petr Mladek 0 siblings, 1 reply; 15+ messages in thread From: Matteo Croce @ 2020-11-03 11:43 UTC (permalink / raw) To: Petr Mladek Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote: > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote: > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote: > > > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > > > > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > > > > deprecated, and restore the old behaviour. > > > > > > > > While at it, merge two identical code blocks into one. > > > > > > > --- a/kernel/reboot.c > > > > +++ b/kernel/reboot.c > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > > > > > > > case 's': > > > > { > > > > - int rc; > > > > - > > > > - if (isdigit(*(str+1))) { > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu); > > > > - if (rc) > > > > - return rc; > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > - reboot_cpu = 0; > > > > - return -ERANGE; > > > > - } > > > > - } else if (str[1] == 'm' && str[2] == 'p' && > > > > - isdigit(*(str+3))) { > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu); > > > > - if (rc) > > > > - return rc; > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > - reboot_cpu = 0; > > > > > > ^^^^^^ > > > > > > > + int cpu; > > > > + > > > > + /* > > > > + * reboot_cpu is s[mp]#### with #### being the processor > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix. > > > > + */ > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > > > > + > > > > + if (isdigit(str[0])) { > > > > + cpu = simple_strtoul(str, NULL, 0); > > > > + if (cpu >= num_possible_cpus()) > > > > return -ERANGE; > > > > - } > > > > + reboot_cpu = cpu; > > > > > > The original value stays when the new one is out of range. It is > > > small functional change that should get mentioned in the commit > > > message or better fixed separately. > > Ah, I see. From some reason, I assumed that it was defined as > module_param() or core_param(). Then it would be possible to modify > it later via /sys. > > I am sorry for the noise. > Never mind :) So, is this an ack? Or I need to prepare a v3 with the revert as first patch? Regards, -- per aspera ad upstream ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-11-03 11:43 ` Matteo Croce @ 2020-11-03 14:25 ` Petr Mladek 2020-11-03 15:43 ` Matteo Croce 0 siblings, 1 reply; 15+ messages in thread From: Petr Mladek @ 2020-11-03 14:25 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt On Tue 2020-11-03 12:43:32, Matteo Croce wrote: > On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote: > > > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote: > > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote: > > > > > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > > > > > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > > > > > deprecated, and restore the old behaviour. > > > > > > > > > > While at it, merge two identical code blocks into one. > > > > > > > > > --- a/kernel/reboot.c > > > > > +++ b/kernel/reboot.c > > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > > > > > > > > > case 's': > > > > > { > > > > > - int rc; > > > > > - > > > > > - if (isdigit(*(str+1))) { > > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu); > > > > > - if (rc) > > > > > - return rc; > > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > > - reboot_cpu = 0; > > > > > - return -ERANGE; > > > > > - } > > > > > - } else if (str[1] == 'm' && str[2] == 'p' && > > > > > - isdigit(*(str+3))) { > > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu); > > > > > - if (rc) > > > > > - return rc; > > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > > - reboot_cpu = 0; > > > > > > > > ^^^^^^ > > > > > > > > > + int cpu; > > > > > + > > > > > + /* > > > > > + * reboot_cpu is s[mp]#### with #### being the processor > > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix. > > > > > + */ > > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > > > > > + > > > > > + if (isdigit(str[0])) { > > > > > + cpu = simple_strtoul(str, NULL, 0); > > > > > + if (cpu >= num_possible_cpus()) > > > > > return -ERANGE; > > > > > - } > > > > > + reboot_cpu = cpu; > > > > > > > > The original value stays when the new one is out of range. It is > > > > small functional change that should get mentioned in the commit > > > > message or better fixed separately. > > > > Ah, I see. From some reason, I assumed that it was defined as > > module_param() or core_param(). Then it would be possible to modify > > it later via /sys. > > > > I am sorry for the noise. > > > > Never mind :) > > So, is this an ack? Or I need to prepare a v3 with the revert as first patch? Good question ;-) It would be nice to do it the cleaner way but I do not resist on it. Feel free to use: Reviewed-by: Petr Mladek <pmladek@suse.com> Now, the question is who would actually push this upstream. These patches often go via Andrew Morton. He actually committed both changes that are fixed here. I suggest to resend the patchset with my Reviewed-by and Cc: stable@vger.kernel.org lines. And put Andrew and Greg into Cc. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-11-03 14:25 ` Petr Mladek @ 2020-11-03 15:43 ` Matteo Croce 2020-11-03 16:22 ` Petr Mladek 0 siblings, 1 reply; 15+ messages in thread From: Matteo Croce @ 2020-11-03 15:43 UTC (permalink / raw) To: Petr Mladek Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt On Tue, Nov 3, 2020 at 3:25 PM Petr Mladek <pmladek@suse.com> wrote: > > On Tue 2020-11-03 12:43:32, Matteo Croce wrote: > > On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote: > > > > > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote: > > > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote: > > > > > > > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > > > > > > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > > > > > > deprecated, and restore the old behaviour. > > > > > > > > > > > > While at it, merge two identical code blocks into one. > > > > > > > > > > > --- a/kernel/reboot.c > > > > > > +++ b/kernel/reboot.c > > > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > > > > > > > > > > > case 's': > > > > > > { > > > > > > - int rc; > > > > > > - > > > > > > - if (isdigit(*(str+1))) { > > > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu); > > > > > > - if (rc) > > > > > > - return rc; > > > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > > > - reboot_cpu = 0; > > > > > > - return -ERANGE; > > > > > > - } > > > > > > - } else if (str[1] == 'm' && str[2] == 'p' && > > > > > > - isdigit(*(str+3))) { > > > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu); > > > > > > - if (rc) > > > > > > - return rc; > > > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > > > - reboot_cpu = 0; > > > > > > > > > > ^^^^^^ > > > > > > > > > > > + int cpu; > > > > > > + > > > > > > + /* > > > > > > + * reboot_cpu is s[mp]#### with #### being the processor > > > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix. > > > > > > + */ > > > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > > > > > > + > > > > > > + if (isdigit(str[0])) { > > > > > > + cpu = simple_strtoul(str, NULL, 0); > > > > > > + if (cpu >= num_possible_cpus()) > > > > > > return -ERANGE; > > > > > > - } > > > > > > + reboot_cpu = cpu; > > > > > > > > > > The original value stays when the new one is out of range. It is > > > > > small functional change that should get mentioned in the commit > > > > > message or better fixed separately. > > > > > > Ah, I see. From some reason, I assumed that it was defined as > > > module_param() or core_param(). Then it would be possible to modify > > > it later via /sys. > > > > > > I am sorry for the noise. > > > > > > > Never mind :) > > > > So, is this an ack? Or I need to prepare a v3 with the revert as first patch? > > Good question ;-) It would be nice to do it the cleaner way but I do > not resist on it. Feel free to use: > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > Now, the question is who would actually push this upstream. These > patches often go via Andrew Morton. He actually committed both > changes that are fixed here. > > I suggest to resend the patchset with my Reviewed-by and > Cc: stable@vger.kernel.org lines. And put Andrew and Greg into Cc. > I see that by doing the revert first, makes the patch very small. It's worth it. I'm thinking, what should be the right action to do when the supplied cpu number is too big? With my patch I stop the parsing, while the previous behavior (other than setting a wrong cpu number) was to continue parsing other fields. Maybe I should just continue the loop and continue the parsing? Maybe with a pr_warn "cpu X exceeds possible cpu number Y" etc. -- per aspera ad upstream ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number 2020-11-03 15:43 ` Matteo Croce @ 2020-11-03 16:22 ` Petr Mladek 0 siblings, 0 replies; 15+ messages in thread From: Petr Mladek @ 2020-11-03 16:22 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt On Tue 2020-11-03 16:43:59, Matteo Croce wrote: > On Tue, Nov 3, 2020 at 3:25 PM Petr Mladek <pmladek@suse.com> wrote: > > > > On Tue 2020-11-03 12:43:32, Matteo Croce wrote: > > > On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@suse.com> wrote: > > > > > > > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote: > > > > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@suse.com> wrote: > > > > > > > > > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote: > > > > > > > From: Matteo Croce <mcroce@microsoft.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, revert the usage of simple_strtoul(), which is no longer > > > > > > > deprecated, and restore the old behaviour. > > > > > > > > > > > > > > While at it, merge two identical code blocks into one. > > > > > > > > > > > > > --- a/kernel/reboot.c > > > > > > > +++ b/kernel/reboot.c > > > > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str) > > > > > > > > > > > > > > case 's': > > > > > > > { > > > > > > > - int rc; > > > > > > > - > > > > > > > - if (isdigit(*(str+1))) { > > > > > > > - rc = kstrtoint(str+1, 0, &reboot_cpu); > > > > > > > - if (rc) > > > > > > > - return rc; > > > > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > > > > - reboot_cpu = 0; > > > > > > > - return -ERANGE; > > > > > > > - } > > > > > > > - } else if (str[1] == 'm' && str[2] == 'p' && > > > > > > > - isdigit(*(str+3))) { > > > > > > > - rc = kstrtoint(str+3, 0, &reboot_cpu); > > > > > > > - if (rc) > > > > > > > - return rc; > > > > > > > - if (reboot_cpu >= num_possible_cpus()) { > > > > > > > - reboot_cpu = 0; > > > > > > > > > > > > ^^^^^^ > > > > > > > > > > > > > + int cpu; > > > > > > > + > > > > > > > + /* > > > > > > > + * reboot_cpu is s[mp]#### with #### being the processor > > > > > > > + * to be used for rebooting. Skip 's' or 'smp' prefix. > > > > > > > + */ > > > > > > > + str += str[1] == 'm' && str[2] == 'p' ? 3 : 1; > > > > > > > + > > > > > > > + if (isdigit(str[0])) { > > > > > > > + cpu = simple_strtoul(str, NULL, 0); > > > > > > > + if (cpu >= num_possible_cpus()) > > > > > > > return -ERANGE; > > > > > > > - } > > > > > > > + reboot_cpu = cpu; > > > > > > > > > > > > The original value stays when the new one is out of range. It is > > > > > > small functional change that should get mentioned in the commit > > > > > > message or better fixed separately. > > > > > > > > Ah, I see. From some reason, I assumed that it was defined as > > > > module_param() or core_param(). Then it would be possible to modify > > > > it later via /sys. > > > > > > > > I am sorry for the noise. > > > > > > > > > > Never mind :) > > > > > > So, is this an ack? Or I need to prepare a v3 with the revert as first patch? > > > > Good question ;-) It would be nice to do it the cleaner way but I do > > not resist on it. Feel free to use: > > > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > > > Now, the question is who would actually push this upstream. These > > patches often go via Andrew Morton. He actually committed both > > changes that are fixed here. > > > > I suggest to resend the patchset with my Reviewed-by and > > Cc: stable@vger.kernel.org lines. And put Andrew and Greg into Cc. > > > > I see that by doing the revert first, makes the patch very small. > It's worth it. Great. > I'm thinking, what should be the right action to do when the supplied > cpu number is too big? > With my patch I stop the parsing, while the previous behavior (other > than setting a wrong cpu number) was to continue parsing other fields. > Maybe I should just continue the loop and continue the parsing? > Maybe with a pr_warn "cpu X exceeds possible cpu number Y" etc. Sounds good to me. Please, make it clear that the error message is printed when parsing reboot= commandline option. For example: pr_err("Ignoring CPU number in reboot= option. CPU X exceeds possible cpu number Y", ...) or something like this. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/2] fix parsing of reboot= cmdline 2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce 2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce 2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce @ 2020-10-27 13:42 ` Greg KH 2 siblings, 0 replies; 15+ messages in thread From: Greg KH @ 2020-10-27 13:42 UTC (permalink / raw) To: Matteo Croce Cc: linux-kernel, Guenter Roeck, Petr Mladek, Arnd Bergmann, Mike Rapoport, Kees Cook, Pavel Tatashin, Robin Holt, Fabian Frederick, stable On Tue, Oct 27, 2020 at 02:35:43PM +0100, Matteo Croce wrote: > From: Matteo Croce <mcroce@microsoft.com> > > The parsing of the reboot= cmdline has two major errors: > - a missing bound check can crash the system on reboot > - parsing of the cpu number only works if specified last > > Fix both, along with a small code refactor. > > v1->v2: > As Petr suggested, don't force base 10 in simple_strtoul(), > so hex values are accepted as well. > > Matteo Croce (2): > reboot: fix overflow parsing reboot cpu number > reboot: fix parsing of reboot cpu number > > kernel/reboot.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > -- > 2.28.0 > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-11-03 16:22 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-27 13:35 [PATCH v2 0/2] fix parsing of reboot= cmdline Matteo Croce 2020-10-27 13:35 ` [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number Matteo Croce 2020-10-27 13:42 ` Greg KH 2020-10-30 14:13 ` Petr Mladek 2020-10-30 14:18 ` Matteo Croce 2020-10-27 13:35 ` [PATCH v2 2/2] reboot: fix parsing of " Matteo Croce 2020-10-27 13:42 ` Greg KH 2020-10-30 14:30 ` Petr Mladek 2020-11-01 1:57 ` Matteo Croce 2020-11-02 11:01 ` Petr Mladek 2020-11-03 11:43 ` Matteo Croce 2020-11-03 14:25 ` Petr Mladek 2020-11-03 15:43 ` Matteo Croce 2020-11-03 16:22 ` Petr Mladek 2020-10-27 13:42 ` [PATCH v2 0/2] fix parsing of reboot= cmdline Greg KH
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).