On 24/04/2019 08:33, Pingfan Liu wrote:
> At present, both return and crash_size should be checked to guarantee the
> success of parse_crashkernel().
>
> Take a close look at the cases, which causes crash_size=0. Beside syntax
> error, three cases cause parsing to get crash_size=0.
> -1st. in parse_crashkernel_mem(), the demanded crash size is bigger than
> system ram.
> -2nd. in parse_crashkernel_mem(), the system ram size does not match any
> item in the range list.
> -3rd. "crashkernel=0MB", which is impractical.
>
> All these cases can be treated as invalid argument.
>
> By this way, only need a simple check on return value of
> parse_crashkernel().
>
> Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Tony Luck <tony.luck@xxxxxxxxx>
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: Paul Burton <paul.burton@xxxxxxxx>
> Cc: James Hogan <jhogan@xxxxxxxxxx>
> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: Paul Mackerras <paulus@xxxxxxxxx>
> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> Cc: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
> Cc: Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
> Cc: Rich Felker <dalias@xxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Julien Thierry <julien.thierry@xxxxxxx>
> Cc: Palmer Dabbelt <palmer@xxxxxxxxxx>
> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> Cc: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Greg Hackmann <ghackmann@xxxxxxxxxxx>
> Cc: Stefan Agner <stefan@xxxxxxxx>
> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxxxxx>
> Cc: Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Hari Bathini <hbathini@xxxxxxxxxxxxx>
> Cc: Ananth N Mavinakayanahalli <ananth@xxxxxxxxxxxxxxxxxx>
> Cc: Yangtao Li <tiny.windzz@xxxxxxxxx>
> Cc: Dave Young <dyoung@xxxxxxxxxx>
> Cc: Baoquan He <bhe@xxxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-ia64@xxxxxxxxxxxxxxx
> Cc: linux-mips@xxxxxxxxxxxxxxx
> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> Cc: linux-s390@xxxxxxxxxxxxxxx
> Cc: linux-sh@xxxxxxxxxxxxxxx
> ---
> v1 -> v2: On error, return -EINVAL for all failure cases
>
> arch/arm/kernel/setup.c | 2 +-
> arch/arm64/mm/init.c | 2 +-
> arch/ia64/kernel/setup.c | 2 +-
> arch/mips/kernel/setup.c | 2 +-
> arch/powerpc/kernel/fadump.c | 2 +-
> arch/powerpc/kernel/machine_kexec.c | 2 +-
> arch/s390/kernel/setup.c | 2 +-
> arch/sh/kernel/machine_kexec.c | 2 +-
> arch/x86/kernel/setup.c | 4 ++--
> kernel/crash_core.c | 10 +++++++++-
> 10 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 5d78b6a..2feab13 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -997,7 +997,7 @@ static void __init reserve_crashkernel(void)
> total_mem = get_total_mem();
> ret = parse_crashkernel(boot_command_line, total_mem,
> &crash_size, &crash_base);
> - if (ret)
> + if (ret < 0)
> return;
>
> if (crash_base <= 0) {
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 6bc1350..240918c 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -79,7 +79,7 @@ static void __init reserve_crashkernel(void)
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> /* no crashkernel= or invalid value specified */
> - if (ret || !crash_size)
> + if (ret < 0)
> return;
>
> crash_size = PAGE_ALIGN(crash_size);
> diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
> index 583a374..3bbb58b 100644
> --- a/arch/ia64/kernel/setup.c
> +++ b/arch/ia64/kernel/setup.c
> @@ -277,7 +277,7 @@ static void __init setup_crashkernel(unsigned long total, int *n)
>
> ret = parse_crashkernel(boot_command_line, total,
> &size, &base);
> - if (ret == 0 && size > 0) {
> + if (!ret) {
> if (!base) {
> sort_regions(rsvd_region, *n);
> *n = merge_regions(rsvd_region, *n);
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 8d1dc6c..168571b 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -715,7 +715,7 @@ static void __init mips_parse_crashkernel(void)
> total_mem = get_total_mem();
> ret = parse_crashkernel(boot_command_line, total_mem,
> &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + if (ret < 0)
> return;
>
> if (!memory_region_available(crash_base, crash_size)) {
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 45a8d0b..3571504 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -376,7 +376,7 @@ static inline unsigned long fadump_calculate_reserve_size(void)
> */
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &size, &base);
> - if (ret == 0 && size > 0) {
> + if (!ret) {
> unsigned long max_size;
>
> if (fw_dump.reserve_bootvar)
> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> index 63f5a93..1697ad2 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -122,7 +122,7 @@ void __init reserve_crashkernel(void)
> /* use common parsing */
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> - if (ret == 0 && crash_size > 0) {
> + if (!ret) {
> crashk_res.start = crash_base;
> crashk_res.end = crash_base + crash_size - 1;
> }
> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index 2c642af..d4bd61b 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
> @@ -671,7 +671,7 @@ static void __init reserve_crashkernel(void)
>
> crash_base = ALIGN(crash_base, KEXEC_CRASH_MEM_ALIGN);
> crash_size = ALIGN(crash_size, KEXEC_CRASH_MEM_ALIGN);
> - if (rc || crash_size == 0)
> + if (rc < 0)
> return;
>
> if (memblock.memory.regions[0].size < crash_size) {
> diff --git a/arch/sh/kernel/machine_kexec.c b/arch/sh/kernel/machine_kexec.c
> index 63d63a3..3c03240 100644
> --- a/arch/sh/kernel/machine_kexec.c
> +++ b/arch/sh/kernel/machine_kexec.c
> @@ -157,7 +157,7 @@ void __init reserve_crashkernel(void)
>
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> - if (ret == 0 && crash_size > 0) {
> + if (!ret) {
> crashk_res.start = crash_base;
> crashk_res.end = crash_base + crash_size - 1;
> }
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 3d872a5..592d5ad 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -526,11 +526,11 @@ static void __init reserve_crashkernel(void)
>
> /* crashkernel=XM */
> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0) {
> + if (ret < 0) {
> /* crashkernel=X,high */
> ret = parse_crashkernel_high(boot_command_line, total_mem,
> &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + if (ret < 0)
> return;
> high = true;
> }
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 093c9f9..83ee4a9 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -108,8 +108,10 @@ static int __init parse_crashkernel_mem(char *cmdline,
> return -EINVAL;
> }
> }
> - } else
> + } else {
> pr_info("crashkernel size resulted in zero bytes\n");
> + return -EINVAL;
> + }
>
> return 0;
> }
> @@ -139,6 +141,8 @@ static int __init parse_crashkernel_simple(char *cmdline,
> pr_warn("crashkernel: unrecognized char: %c\n", *cur);
> return -EINVAL;
> }
> + if (*crash_size == 0)
> + return -EINVAL;
This covers the case where I pass an argument like "crashkernel=0M" ?
Can't we fix that by using kstrtoull() in memparse and check if the return value
is < 0? In that case we could return without updating the retptr and we will be
fine.
>
> return 0;
> }
> @@ -181,6 +185,8 @@ static int __init parse_crashkernel_suffix(char *cmdline,
> pr_warn("crashkernel: unrecognized char: %c\n", *cur);
> return -EINVAL;
> }
> + if (*crash_size == 0)
> + return -EINVAL;
Same here.
>
> return 0;
> }
> @@ -266,6 +272,8 @@ static int __init __parse_crashkernel(char *cmdline,
> /*
> * That function is the entry point for command line parsing and should be
> * called from the arch-specific code.
> + * On success 0. On error for either syntax error or crash_size=0, -EINVAL is
> + * returned.
> */
> int __init parse_crashkernel(char *cmdline,
> unsigned long long system_ram,
>
