fix parsing of reboot= cmdline
v2 cover fix parsing of reboot= cmdline
From: Matteo Croce <[email protected]>
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(-)
Signature · 1 line
2.28.0
PATCH v2 1/2 reboot: fix overflow parsing reboot cpu number
From: Matteo Croce <[email protected]>
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 ]------ kernel/reboot.c | 8 ++++++++ 1 file changed, 8 insertions(+)
kernel/reboot.c +8 -0
@@ -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 ;
Signature · 1 line
2.28.0
Greg KH
maintainerRe: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
Quoted 59 lines · Matteo Croce
From: Matteo Croce <[email protected]>
Limit the CPU number to
num_possible_cpus(), because setting it to a value lower thanINT_MAXbut higher thanNR_CPUSproduces 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 ]---
Quoted 6 lines
Fixes 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")Signed-off-by Matteo Croce <[email protected]>--- 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>
Petr Mladek
Reviewed-byRe: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
Quoted 9 lines · Greg KH
Quoted 8 lines · Matteo Croce
From: Matteo Croce <[email protected]>
Limit the CPU number to
num_possible_cpus(), because setting it to a value lower thanINT_MAXbut higher thanNR_CPUSproduces the following error on reboot and shutdown:Fixes 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")Signed-off-by Matteo Croce <[email protected]>
Quoted 8 lines
<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:
Best Regards,
Petr
Re: [PATCH v2 1/2] reboot: fix overflow parsing reboot cpu number
Quoted 21 lines · Petr Mladek
Quoted 9 lines · Greg KH
Quoted 8 lines · Matteo Croce
From: Matteo Croce <[email protected]>
Limit the CPU number to
num_possible_cpus(), because setting it to a value lower thanINT_MAXbut higher thanNR_CPUSproduces the following error on reboot and shutdown:Fixes 1b3a5d02ee07 ("reboot: move arch/x86 reboot= handling to generic kernel")Signed-off-by Matteo Croce <[email protected]>Reviewed-by Petr Mladek <[email protected]>Quoted 7 lines
<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.
Quoted 6 lines
Quoted 1 line
</formletter>
The best way is to add the following line before Signed-off-by line:
I see, sorry for the noise. Should I resend, or I eventually write to [email protected] after the merge?
Signature · 1 line
per aspera ad upstream
PATCH v2 2/2 reboot: fix parsing of reboot cpu number
In reply to Matteo CroceFrom: Matteo Croce <[email protected]>
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.
--- kernel/reboot.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
kernel/reboot.c +12 -18
@@ -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 ;
Signature · 1 line
2.28.0
Greg KH
maintainerRe: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 18 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
Quoted 8 lines
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 <[email protected]>--- 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>
Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
In reply to Matteo CroceQuoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {
kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;
^^^^^^
Quoted 4 lines
kernel/reboot.c +4
-0+int cpu; + +/* +* reboot_cpu is s[mp]#### with #### being the processor
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 10 lines
kernel/reboot.c +8
-1+* 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
Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 58 lines · Petr Mladek
Quoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;^^^^^^Quoted 10 lines
kernel/reboot.c +10
-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 );
Quoted 9 lines
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 4 lines
kernel/reboot.c +2
-1+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)
Quoted 5 lines
Hmm, I suggest to split this into 3 patches and switch the order:
+ 1st patch should simply revert the commit
616feab75397("kernel/reboot.c: convertsimple_strtoulto kstrtoint").
Quoted 9 lines
+ 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
Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 59 lines · Matteo Croce
Quoted 58 lines · Petr Mladek
Quoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;^^^^^^Quoted 10 lines
kernel/reboot.c +10
-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 );
Quoted 8 lines
Quoted 8 lines
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 4 lines
kernel/reboot.c +2
-1+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
Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 70 lines · Petr Mladek
Quoted 59 lines · Matteo Croce
Quoted 58 lines · Petr Mladek
Quoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;^^^^^^Quoted 10 lines
kernel/reboot.c +10
-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 );Quoted 8 lines
Quoted 8 lines
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 4 lines
kernel/reboot.c +2
-1+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.
Quoted 6 lines
Ah, I see. From some reason, I assumed that it was defined as
module_param()orcore_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,
Signature · 1 line
per aspera ad upstream
Petr Mladek
Reviewed-byRe: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 75 lines · Matteo Croce
Quoted 74 lines · Petr Mladek
Quoted 59 lines · Matteo Croce
Quoted 58 lines · Petr Mladek
Quoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;^^^^^^Quoted 10 lines
kernel/reboot.c +10
-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 );Quoted 8 lines
Quoted 8 lines
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 4 lines
kernel/reboot.c +2
-1+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()orcore_param(). Then it would be possible to modify it later via /sys.
Quoted 6 lines
Quoted 2 lines
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:
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: [email protected] lines. And put Andrew and Greg into Cc.
Best Regards,
Petr
Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 87 lines · Petr Mladek
Quoted 75 lines · Matteo Croce
Quoted 74 lines · Petr Mladek
Quoted 59 lines · Matteo Croce
Quoted 58 lines · Petr Mladek
Quoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;^^^^^^Quoted 10 lines
kernel/reboot.c +10
-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 );Quoted 8 lines
Quoted 8 lines
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 4 lines
kernel/reboot.c +2
-1+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()orcore_param(). Then it would be possible to modify it later via /sys.Quoted 6 lines
Quoted 2 lines
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:
Quoted 9 lines
Reviewed-by Petr Mladek <[email protected]>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: [email protected] 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.
Signature · 1 line
per aspera ad upstream
Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number
Quoted 94 lines · Matteo Croce
Quoted 93 lines · Petr Mladek
Quoted 75 lines · Matteo Croce
Quoted 74 lines · Petr Mladek
Quoted 59 lines · Matteo Croce
Quoted 58 lines · Petr Mladek
Quoted 33 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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,forceIn the early days the parsing was done with
simple_strtoul(), later deprecated in favor of the saferkstrtoint()which handles overflow.But
kstrtoint()returns-EINVALif 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.
kernel/reboot.c +0
-7@@ -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 ()) {kernel/reboot.c static int __init reboot_setup(char *str) · line 571
Quoted 10 lines
kernel/reboot.c +0
-10- 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 ;^^^^^^Quoted 10 lines
kernel/reboot.c +10
-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 );Quoted 8 lines
Quoted 8 lines
kernel/reboot.c static int __init reboot_setup(char *str) · line 567
Quoted 4 lines
kernel/reboot.c +2
-1+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()orcore_param(). Then it would be possible to modify it later via /sys.Quoted 6 lines
Quoted 2 lines
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 <[email protected]>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.
Quoted 6 lines
Quoted 3 lines
I suggest to resend the patchset with my Reviewed-by and
Cc: [email protected] 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.
Quoted 6 lines
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 apr_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
Greg KH
maintainerQuoted 12 lines · Matteo Croce
From: Matteo Croce <[email protected]>
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 lastFix both, along with a small code refactor.
v1->v2:
As Petr suggested, don't force base 10 insimple_strtoul(), so hex values are accepted as well.
Quoted 10 lines
Matteo Croce (2):
reboot: fix overflow parsing reboot cpu number
reboot: fix parsing of reboot cpu numberkernel/reboot.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)Signature · 1 line
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>