| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 24 Apr 2019 10:31:37 +0200
From: Matthias Brugger <mbrugger@...e.com>
To: Pingfan Liu <kernelfans@...il.com>, linux-kernel@...r.kernel.org
Cc: Rich Felker <dalias@...c.org>, linux-ia64@...r.kernel.org,
Julien Thierry <julien.thierry@....com>,
Yangtao Li <tiny.windzz@...il.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Palmer Dabbelt <palmer@...ive.com>,
Heiko Carstens <heiko.carstens@...ibm.com>, x86@...nel.org,
linux-mips@...r.kernel.org, Paul Mackerras <paulus@...ba.org>,
"H. Peter Anvin" <hpa@...or.com>, linux-s390@...r.kernel.org,
Florian Fainelli <f.fainelli@...il.com>,
Yoshinori Sato <ysato@...rs.sourceforge.jp>,
linux-sh@...r.kernel.org, Michael Ellerman <mpe@...erman.id.au>,
David Hildenbrand <david@...hat.com>,
Russell King <linux@...linux.org.uk>,
Ingo Molnar <mingo@...hat.com>,
linux-arm-kernel@...ts.infradead.org,
Catalin Marinas <catalin.marinas@....com>,
James Hogan <jhogan@...nel.org>,
Dave Young <dyoung@...hat.com>,
Fenghua Yu <fenghua.yu@...el.com>,
Will Deacon <will.deacon@....com>,
linuxppc-dev@...ts.ozlabs.org,
Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
Borislav Petkov <bp@...en8.de>, Stefan Agner <stefan@...er.ch>,
Thomas Gleixner <tglx@...utronix.de>,
Hari Bathini <hbathini@...ux.ibm.com>,
Jens Axboe <axboe@...nel.dk>, Tony Luck <tony.luck@...el.com>,
Baoquan He <bhe@...hat.com>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Robin Murphy <robin.murphy@....com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Ralf Baechle <ralf@...ux-mips.org>,
Thomas Bogendoerfer <tbogendoerfer@...e.de>,
Paul Burton <paul.burton@...s.com>,
Johannes Weiner <hannes@...xchg.org>,
Martin Schwidefsky <schwidefsky@...ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Logan Gunthorpe <logang@...tatee.com>,
Greg Hackmann <ghackmann@...roid.com>
Subject: Re: [PATCHv2] kernel/crash: make parse_crashkernel()'s return value
more indicant
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@...il.com>
> Cc: Russell King <linux@...linux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Will Deacon <will.deacon@....com>
> Cc: Tony Luck <tony.luck@...el.com>
> Cc: Fenghua Yu <fenghua.yu@...el.com>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: Paul Burton <paul.burton@...s.com>
> Cc: James Hogan <jhogan@...nel.org>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Cc: Paul Mackerras <paulus@...ba.org>
> Cc: Michael Ellerman <mpe@...erman.id.au>
> Cc: Martin Schwidefsky <schwidefsky@...ibm.com>
> Cc: Heiko Carstens <heiko.carstens@...ibm.com>
> Cc: Yoshinori Sato <ysato@...rs.sourceforge.jp>
> Cc: Rich Felker <dalias@...c.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Julien Thierry <julien.thierry@....com>
> Cc: Palmer Dabbelt <palmer@...ive.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> Cc: Florian Fainelli <f.fainelli@...il.com>
> Cc: Logan Gunthorpe <logang@...tatee.com>
> Cc: Robin Murphy <robin.murphy@....com>
> Cc: Greg Hackmann <ghackmann@...roid.com>
> Cc: Stefan Agner <stefan@...er.ch>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: David Hildenbrand <david@...hat.com>
> Cc: Jens Axboe <axboe@...nel.dk>
> Cc: Thomas Bogendoerfer <tbogendoerfer@...e.de>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Hari Bathini <hbathini@...ux.ibm.com>
> Cc: Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>
> Cc: Yangtao Li <tiny.windzz@...il.com>
> Cc: Dave Young <dyoung@...hat.com>
> Cc: Baoquan He <bhe@...hat.com>
> Cc: x86@...nel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-ia64@...r.kernel.org
> Cc: linux-mips@...r.kernel.org
> Cc: linuxppc-dev@...ts.ozlabs.org
> Cc: linux-s390@...r.kernel.org
> Cc: linux-sh@...r.kernel.org
> ---
> 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,
>
Powered by blists - more mailing lists