| 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
| ||
|
Message-ID: <875za77o05.fsf@mpe.ellerman.id.au>
Date: Tue, 28 Jul 2020 23:44:26 +1000
From: Michael Ellerman <mpe@...erman.id.au>
To: Hari Bathini <hbathini@...ux.ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Pingfan Liu <piliu@...hat.com>,
Mahesh J Salgaonkar <mahesh@...ux.ibm.com>,
Sourabh Jain <sourabhjain@...ux.ibm.com>,
Vivek Goyal <vgoyal@...hat.com>,
Dave Young <dyoung@...hat.com>,
Petr Tesarik <ptesarik@...e.cz>,
Pingfan Liu <piliu@...hat.com>,
linuxppc-dev <linuxppc-dev@...abs.org>,
Kexec-ml <kexec@...ts.infradead.org>,
lkml <linux-kernel@...r.kernel.org>,
Pingfan Liu <piliu@...hat.com>,
Eric Biederman <ebiederm@...ssion.com>,
Thiago Jung Bauermann <bauerman@...ux.ibm.com>,
Mimi Zohar <zohar@...ux.ibm.com>,
Nayna Jain <nayna@...ux.ibm.com>
Subject: Re: [RESEND PATCH v5 06/11] ppc64/kexec_file: restrict memory usage of kdump kernel
Hari Bathini <hbathini@...ux.ibm.com> writes:
> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
> index 2df6f4273ddd..8df085a22fd7 100644
> --- a/arch/powerpc/kexec/file_load_64.c
> +++ b/arch/powerpc/kexec/file_load_64.c
> @@ -17,9 +17,21 @@
> #include <linux/kexec.h>
> #include <linux/of_fdt.h>
> #include <linux/libfdt.h>
> +#include <linux/of_device.h>
> #include <linux/memblock.h>
> +#include <linux/slab.h>
> +#include <asm/drmem.h>
> #include <asm/kexec_ranges.h>
>
> +struct umem_info {
> + uint64_t *buf; /* data buffer for usable-memory property */
> + uint32_t idx; /* current index */
> + uint32_t size; /* size allocated for the data buffer */
Use kernel types please, u64, u32.
> + /* usable memory ranges to look up */
> + const struct crash_mem *umrngs;
"umrngs".
Given it's part of the umem_info struct could it just be "ranges"?
> +};
> +
> const struct kexec_file_ops * const kexec_file_loaders[] = {
> &kexec_elf64_ops,
> NULL
> @@ -74,6 +86,42 @@ static int get_exclude_memory_ranges(struct crash_mem **mem_ranges)
> return ret;
> }
>
> +/**
> + * get_usable_memory_ranges - Get usable memory ranges. This list includes
> + * regions like crashkernel, opal/rtas & tce-table,
> + * that kdump kernel could use.
> + * @mem_ranges: Range list to add the memory ranges to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +static int get_usable_memory_ranges(struct crash_mem **mem_ranges)
> +{
> + int ret;
> +
> + /*
> + * prom code doesn't take kindly to missing low memory. So, add
I don't know what that's referring to, "prom code" is too vague.
> + * [0, crashk_res.end] instead of [crashk_res.start, crashk_res.end]
> + * to keep it happy.
> + */
> + ret = add_mem_range(mem_ranges, 0, crashk_res.end + 1);
> + if (ret)
> + goto out;
> +
> + ret = add_rtas_mem_range(mem_ranges);
> + if (ret)
> + goto out;
> +
> + ret = add_opal_mem_range(mem_ranges);
> + if (ret)
> + goto out;
> +
> + ret = add_tce_mem_ranges(mem_ranges);
> +out:
> + if (ret)
> + pr_err("Failed to setup usable memory ranges\n");
> + return ret;
> +}
> +
> /**
> * __locate_mem_hole_top_down - Looks top down for a large enough memory hole
> * in the memory regions between buf_min & buf_max
> @@ -273,6 +321,382 @@ static int locate_mem_hole_bottom_up_ppc64(struct kexec_buf *kbuf,
> return ret;
> }
>
> +/**
> + * check_realloc_usable_mem - Reallocate buffer if it can't accommodate entries
> + * @um_info: Usable memory buffer and ranges info.
> + * @cnt: No. of entries to accommodate.
> + *
> + * Frees up the old buffer if memory reallocation fails.
> + *
> + * Returns buffer on success, NULL on error.
> + */
> +static uint64_t *check_realloc_usable_mem(struct umem_info *um_info, int cnt)
> +{
> + void *tbuf;
> +
> + if (um_info->size >=
> + ((um_info->idx + cnt) * sizeof(*(um_info->buf))))
> + return um_info->buf;
This is awkward.
AFAICS you only use um_info->size here, so instead why not store the
number of u64s you have space for, as num for example.
Then the above comparison becomes:
if (um_info->num >= (um_info->idx + count))
Then you only have to calculate the size internally here for the
realloc.
> +
> + um_info->size += MEM_RANGE_CHUNK_SZ;
new_size = um_info->size + MEM_RANGE_CHUNK_SZ;
tbuf = krealloc(um_info->buf, new_size, GFP_KERNEL);
> + tbuf = krealloc(um_info->buf, um_info->size, GFP_KERNEL);
> + if (!tbuf) {
> + um_info->size -= MEM_RANGE_CHUNK_SZ;
Then you can drop this.
> + return NULL;
> + }
um_info->size = new_size;
> +
> + memset(tbuf + um_info->idx, 0, MEM_RANGE_CHUNK_SZ);
Just pass __GFP_ZERO to krealloc?
> + return tbuf;
> +}
> +
> +/**
> + * add_usable_mem - Add the usable memory ranges within the given memory range
> + * to the buffer
> + * @um_info: Usable memory buffer and ranges info.
> + * @base: Base address of memory range to look for.
> + * @end: End address of memory range to look for.
> + * @cnt: No. of usable memory ranges added to buffer.
One caller never uses this AFAICS.
Couldn't the other caller just compare the um_info->idx before and after
the call, and avoid another pass by reference parameter.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +static int add_usable_mem(struct umem_info *um_info, uint64_t base,
> + uint64_t end, int *cnt)
> +{
> + uint64_t loc_base, loc_end, *buf;
> + const struct crash_mem *umrngs;
> + int i, add;
add should be bool.
> + *cnt = 0;
> + umrngs = um_info->umrngs;
> + for (i = 0; i < umrngs->nr_ranges; i++) {
> + add = 0;
> + loc_base = umrngs->ranges[i].start;
> + loc_end = umrngs->ranges[i].end;
> + if (loc_base >= base && loc_end <= end)
> + add = 1;
> + else if (base < loc_end && end > loc_base) {
> + if (loc_base < base)
> + loc_base = base;
> + if (loc_end > end)
> + loc_end = end;
> + add = 1;
> + }
> +
> + if (add) {
> + buf = check_realloc_usable_mem(um_info, 2);
> + if (!buf)
> + return -ENOMEM;
> +
> + um_info->buf = buf;
> + buf[um_info->idx++] = cpu_to_be64(loc_base);
> + buf[um_info->idx++] =
> + cpu_to_be64(loc_end - loc_base + 1);
> + (*cnt)++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * kdump_setup_usable_lmb - This is a callback function that gets called by
> + * walk_drmem_lmbs for every LMB to set its
> + * usable memory ranges.
> + * @lmb: LMB info.
> + * @usm: linux,drconf-usable-memory property value.
> + * @data: Pointer to usable memory buffer and ranges info.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +static int kdump_setup_usable_lmb(struct drmem_lmb *lmb, const __be32 **usm,
> + void *data)
> +{
> + struct umem_info *um_info;
> + uint64_t base, end, *buf;
> + int cnt, tmp_idx, ret;
> +
> + /*
> + * kdump load isn't supported on kernels already booted with
> + * linux,drconf-usable-memory property.
> + */
> + if (*usm) {
> + pr_err("linux,drconf-usable-memory property already exists!");
> + return -EINVAL;
> + }
> +
> + um_info = data;
> + tmp_idx = um_info->idx;
> + buf = check_realloc_usable_mem(um_info, 1);
> + if (!buf)
> + return -ENOMEM;
> +
> + um_info->idx++;
> + um_info->buf = buf;
> + base = lmb->base_addr;
> + end = base + drmem_lmb_size() - 1;
> + ret = add_usable_mem(um_info, base, end, &cnt);
> + if (!ret)
> + um_info->buf[tmp_idx] = cpu_to_be64(cnt);
> +
> + return ret;
> +}
> +
> +/**
> + * get_node_path_size - Get the full path length of the given node.
> + * @dn: Device Node.
> + *
> + * Also, counts '\0' at the end of the path.
> + * For example, /memory@0 will be "/memory@0\0" => 10 bytes.
> + *
> + * Returns the string size of the node's full path.
> + */
> +static int get_node_path_size(struct device_node *dn)
> +{
> + int len = 0;
> +
> + if (!dn)
> + return 0;
> +
> + /* Root node */
> + if (!(dn->parent))
> + return 2;
> +
> + while (dn) {
> + len += strlen(dn->full_name) + 1;
> + dn = dn->parent;
> + }
> +
> + return len;
> +}
> +
> +/**
> + * get_node_path - Get the full path of the given node.
> + * @node: Device node.
> + *
> + * Allocates buffer for node path. The caller must free the buffer
> + * after use.
> + *
> + * Returns buffer with path on success, NULL otherwise.
> + */
> +static char *get_node_path(struct device_node *node)
> +{
As discussed this can probably be replaced with snprintf(buf, "%pOF") ?
cheers
Powered by blists - more mailing lists