diff mbox series

[v2,1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line

Message ID 1537628013-243902-1-git-send-email-zhe.he@windriver.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] mm/page_alloc: Fix panic caused by passing debug_guardpage_minorder or kernelcore to command line | expand

Commit Message

He Zhe Sept. 22, 2018, 2:53 p.m. UTC
From: He Zhe <zhe.he@windriver.com>

debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
input argument before using it. The argument would be a NULL pointer if
"debug_guardpage_minorder" or "kernelcore", without its value, is set in
command line and thus causes the following panic.

PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
[    0.000000] RIP: 0010:parse_option_str+0x11/0x90
...
[    0.000000] Call Trace:
[    0.000000]  cmdline_parse_kernelcore+0x19/0x41
[    0.000000]  do_early_param+0x57/0x8e
[    0.000000]  parse_args+0x208/0x320
[    0.000000]  ? rdinit_setup+0x30/0x30
[    0.000000]  parse_early_options+0x29/0x2d
[    0.000000]  ? rdinit_setup+0x30/0x30
[    0.000000]  parse_early_param+0x36/0x4d
[    0.000000]  setup_arch+0x336/0x99e
[    0.000000]  start_kernel+0x6f/0x4ee
[    0.000000]  x86_64_start_reservations+0x24/0x26
[    0.000000]  x86_64_start_kernel+0x6f/0x72
[    0.000000]  secondary_startup_64+0xa4/0xb0

This patch adds a check to prevent the panic and adds KBUILD_MODNAME to
prints.

Signed-off-by: He Zhe <zhe.he@windriver.com>
Cc: stable@vger.kernel.org
Cc: akpm@linux-foundation.org
Cc: mhocko@suse.com
Cc: vbabka@suse.cz
Cc: pasha.tatashin@oracle.com
Cc: mgorman@techsingularity.net
Cc: aaron.lu@intel.com
Cc: osalvador@suse.de
Cc: iamjoonsoo.kim@lge.com
---
v2:
Use more clear error info
Split the addition of KBUILD_MODNAME out

 mm/page_alloc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Michal Hocko Sept. 24, 2018, 2:24 p.m. UTC | #1
On Sat 22-09-18 22:53:32, zhe.he@windriver.com wrote:
> From: He Zhe <zhe.he@windriver.com>
> 
> debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
> input argument before using it. The argument would be a NULL pointer if
> "debug_guardpage_minorder" or "kernelcore", without its value, is set in
> command line and thus causes the following panic.
> 
> PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
> [    0.000000] RIP: 0010:parse_option_str+0x11/0x90
> ...
> [    0.000000] Call Trace:
> [    0.000000]  cmdline_parse_kernelcore+0x19/0x41
> [    0.000000]  do_early_param+0x57/0x8e
> [    0.000000]  parse_args+0x208/0x320
> [    0.000000]  ? rdinit_setup+0x30/0x30
> [    0.000000]  parse_early_options+0x29/0x2d
> [    0.000000]  ? rdinit_setup+0x30/0x30
> [    0.000000]  parse_early_param+0x36/0x4d
> [    0.000000]  setup_arch+0x336/0x99e
> [    0.000000]  start_kernel+0x6f/0x4ee
> [    0.000000]  x86_64_start_reservations+0x24/0x26
> [    0.000000]  x86_64_start_kernel+0x6f/0x72
> [    0.000000]  secondary_startup_64+0xa4/0xb0
> 
> This patch adds a check to prevent the panic

Is this something we deeply care about? The kernel command line
interface is to be used by admins who know what they are doing.  Using
random or wrong values for these parameters can have detrimental effects
on the system. This particular case would blow up early, good. At least
it is visible immediately. This and many other parameters could have a
seemingly valid input (e.g. not a missing value) and subtle runtime
effect. You won't blow up immediately but the system is hardly usable
and the early checking cannot possible catch all those cases. Take a
mem=$N copied from one machine to another with a different memory
layout. While 2G can be perfectly fine on one a different machine might
result on a completely unusable system because the available RAM is
place higher.

So I am really wondering. Do we really want a lot of code to catch
kernel command line incorrect inputs? Does it really lead to better
quality overall? IMHO, we do have a proper documentation and we should
trust those starting the kernel.

> and adds KBUILD_MODNAME to prints.

This doesn't seem to be done in this patch. Probably a left over
from the previous version.

> Signed-off-by: He Zhe <zhe.he@windriver.com>
> Cc: stable@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Cc: mhocko@suse.com
> Cc: vbabka@suse.cz
> Cc: pasha.tatashin@oracle.com
> Cc: mgorman@techsingularity.net
> Cc: aaron.lu@intel.com
> Cc: osalvador@suse.de
> Cc: iamjoonsoo.kim@lge.com
> ---
> v2:
> Use more clear error info
> Split the addition of KBUILD_MODNAME out
> 
>  mm/page_alloc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 89d2a2a..f34cae1 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -630,6 +630,12 @@ static int __init debug_guardpage_minorder_setup(char *buf)
>  {
>  	unsigned long res;
>  
> +	if (!buf) {
> +		pr_err("kernel option debug_guardpage_minorder requires an \
> +			argument\n");
> +		return -EINVAL;
> +	}
> +
>  	if (kstrtoul(buf, 10, &res) < 0 ||  res > MAX_ORDER / 2) {
>  		pr_err("Bad debug_guardpage_minorder value\n");
>  		return 0;
> @@ -6952,6 +6958,11 @@ static int __init cmdline_parse_core(char *p, unsigned long *core,
>   */
>  static int __init cmdline_parse_kernelcore(char *p)
>  {
> +	if (!p) {
> +		pr_err("kernel option kernelcore requires an argument\n");
> +		return -EINVAL;
> +	}
> +
>  	/* parse kernelcore=mirror */
>  	if (parse_option_str(p, "mirror")) {
>  		mirrored_kernelcore = true;
> -- 
> 2.7.4
>
Andrew Morton Sept. 24, 2018, 9:42 p.m. UTC | #2
On Mon, 24 Sep 2018 16:24:08 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> On Sat 22-09-18 22:53:32, zhe.he@windriver.com wrote:
> > From: He Zhe <zhe.he@windriver.com>
> > 
> > debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
> > input argument before using it. The argument would be a NULL pointer if
> > "debug_guardpage_minorder" or "kernelcore", without its value, is set in
> > command line and thus causes the following panic.
> > 
> > PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
> > [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
> > [    0.000000] RIP: 0010:parse_option_str+0x11/0x90
> > ...
> > [    0.000000] Call Trace:
> > [    0.000000]  cmdline_parse_kernelcore+0x19/0x41
> > [    0.000000]  do_early_param+0x57/0x8e
> > [    0.000000]  parse_args+0x208/0x320
> > [    0.000000]  ? rdinit_setup+0x30/0x30
> > [    0.000000]  parse_early_options+0x29/0x2d
> > [    0.000000]  ? rdinit_setup+0x30/0x30
> > [    0.000000]  parse_early_param+0x36/0x4d
> > [    0.000000]  setup_arch+0x336/0x99e
> > [    0.000000]  start_kernel+0x6f/0x4ee
> > [    0.000000]  x86_64_start_reservations+0x24/0x26
> > [    0.000000]  x86_64_start_kernel+0x6f/0x72
> > [    0.000000]  secondary_startup_64+0xa4/0xb0
> > 
> > This patch adds a check to prevent the panic
> 
> Is this something we deeply care about? The kernel command line
> interface is to be used by admins who know what they are doing.  Using
> random or wrong values for these parameters can have detrimental effects
> on the system. This particular case would blow up early, good. At least
> it is visible immediately. This and many other parameters could have a
> seemingly valid input (e.g. not a missing value) and subtle runtime
> effect. You won't blow up immediately but the system is hardly usable
> and the early checking cannot possible catch all those cases. Take a
> mem=$N copied from one machine to another with a different memory
> layout. While 2G can be perfectly fine on one a different machine might
> result on a completely unusable system because the available RAM is
> place higher.
> 
> So I am really wondering. Do we really want a lot of code to catch
> kernel command line incorrect inputs? Does it really lead to better
> quality overall? IMHO, we do have a proper documentation and we should
> trust those starting the kernel.

No, it's not very important.  It might help some people understand why
their kernel went splat in rare circumstances.  And it's __init code so
the runtime impact is nil.

It bothers me that there are many other kernel parameters which have
the same undesirable behaviour.  I'd much prefer a general fixup which
gave all of them this treatment, but it's unclear how to do this.
Michal Hocko Sept. 25, 2018, 5:59 a.m. UTC | #3
On Mon 24-09-18 14:42:17, Andrew Morton wrote:
> On Mon, 24 Sep 2018 16:24:08 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > On Sat 22-09-18 22:53:32, zhe.he@windriver.com wrote:
> > > From: He Zhe <zhe.he@windriver.com>
> > > 
> > > debug_guardpage_minorder_setup and cmdline_parse_kernelcore do not check
> > > input argument before using it. The argument would be a NULL pointer if
> > > "debug_guardpage_minorder" or "kernelcore", without its value, is set in
> > > command line and thus causes the following panic.
> > > 
> > > PANIC: early exception 0xe3 IP 10:ffffffffa08146f1 error 0 cr2 0x0
> > > [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #11
> > > [    0.000000] RIP: 0010:parse_option_str+0x11/0x90
> > > ...
> > > [    0.000000] Call Trace:
> > > [    0.000000]  cmdline_parse_kernelcore+0x19/0x41
> > > [    0.000000]  do_early_param+0x57/0x8e
> > > [    0.000000]  parse_args+0x208/0x320
> > > [    0.000000]  ? rdinit_setup+0x30/0x30
> > > [    0.000000]  parse_early_options+0x29/0x2d
> > > [    0.000000]  ? rdinit_setup+0x30/0x30
> > > [    0.000000]  parse_early_param+0x36/0x4d
> > > [    0.000000]  setup_arch+0x336/0x99e
> > > [    0.000000]  start_kernel+0x6f/0x4ee
> > > [    0.000000]  x86_64_start_reservations+0x24/0x26
> > > [    0.000000]  x86_64_start_kernel+0x6f/0x72
> > > [    0.000000]  secondary_startup_64+0xa4/0xb0
> > > 
> > > This patch adds a check to prevent the panic
> > 
> > Is this something we deeply care about? The kernel command line
> > interface is to be used by admins who know what they are doing.  Using
> > random or wrong values for these parameters can have detrimental effects
> > on the system. This particular case would blow up early, good. At least
> > it is visible immediately. This and many other parameters could have a
> > seemingly valid input (e.g. not a missing value) and subtle runtime
> > effect. You won't blow up immediately but the system is hardly usable
> > and the early checking cannot possible catch all those cases. Take a
> > mem=$N copied from one machine to another with a different memory
> > layout. While 2G can be perfectly fine on one a different machine might
> > result on a completely unusable system because the available RAM is
> > place higher.
> > 
> > So I am really wondering. Do we really want a lot of code to catch
> > kernel command line incorrect inputs? Does it really lead to better
> > quality overall? IMHO, we do have a proper documentation and we should
> > trust those starting the kernel.
> 
> No, it's not very important.  It might help some people understand why
> their kernel went splat in rare circumstances.  And it's __init code so
> the runtime impact is nil.
> 
> It bothers me that there are many other kernel parameters which have
> the same undesirable behaviour.  I'd much prefer a general fixup which
> gave all of them this treatment, but it's unclear how to do this.

If early_param took an additional argument to tell "this really requires
a parameter" then we could do it in the common code.

$ git grep "early_param(\"" | wc -l
251

quite a lot of work for something that hasn't been a problem for years I
guess. But maybe this would allow to remove ad-hoc checks in handlers
and reduce the overal code size (in LOC) in the end.
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 89d2a2a..f34cae1 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -630,6 +630,12 @@  static int __init debug_guardpage_minorder_setup(char *buf)
 {
 	unsigned long res;
 
+	if (!buf) {
+		pr_err("kernel option debug_guardpage_minorder requires an \
+			argument\n");
+		return -EINVAL;
+	}
+
 	if (kstrtoul(buf, 10, &res) < 0 ||  res > MAX_ORDER / 2) {
 		pr_err("Bad debug_guardpage_minorder value\n");
 		return 0;
@@ -6952,6 +6958,11 @@  static int __init cmdline_parse_core(char *p, unsigned long *core,
  */
 static int __init cmdline_parse_kernelcore(char *p)
 {
+	if (!p) {
+		pr_err("kernel option kernelcore requires an argument\n");
+		return -EINVAL;
+	}
+
 	/* parse kernelcore=mirror */
 	if (parse_option_str(p, "mirror")) {
 		mirrored_kernelcore = true;