linux-kernel - Re: [PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875zad6ajx.fsf@morokweng.localdomain>
Date:   Thu, 23 Jul 2020 21:06:42 -0300
From:   Thiago Jung Bauermann <bauerman@...ux.ibm.com>
To:     Hari Bathini <hbathini@...ux.ibm.com>
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Pingfan Liu <piliu@...hat.com>,
        Kexec-ml <kexec@...ts.infradead.org>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Nayna Jain <nayna@...ux.ibm.com>,
        Petr Tesarik <ptesarik@...e.cz>,
        Mahesh J Salgaonkar <mahesh@...ux.ibm.com>,
        Sourabh Jain <sourabhjain@...ux.ibm.com>,
        lkml <linux-kernel@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...abs.org>,
        Eric Biederman <ebiederm@...ssion.com>,
        Dave Young <dyoung@...hat.com>, Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel


Hari Bathini <hbathini@...ux.ibm.com> writes:

> Kdump kernel, used for capturing the kernel core image, is supposed
> to use only specific memory regions to avoid corrupting the image to
> be captured. The regions are crashkernel range - the memory reserved
> explicitly for kdump kernel, memory used for the tce-table, the OPAL
> region and RTAS region as applicable. Restrict kdump kernel memory
> to use only these regions by setting up usable-memory DT property.
> Also, tell the kdump kernel to run at the loaded address by setting
> the magic word at 0x5c.
>
> Signed-off-by: Hari Bathini <hbathini@...ux.ibm.com>
> Tested-by: Pingfan Liu <piliu@...hat.com>
> ---
>
> v3 -> v4:
> * Updated get_node_path() to be an iterative function instead of a
>   recursive one.
> * Added comment explaining why low memory is added to kdump kernel's
>   usable memory ranges though it doesn't fall in crashkernel region.
> * For correctness, added fdt_add_mem_rsv() for the low memory being
>   added to kdump kernel's usable memory ranges.

Good idea.

> * Fixed prop pointer update in add_usable_mem_property() and changed
>   duple to tuple as suggested by Thiago.

<snip>

> +/**
> + * get_node_pathlen - Get the full path length of the given node.
> + * @dn:               Node.
> + *
> + * Also, counts '/' at the end of the path.
> + * For example, /memory@0 will be "/memory@0/\0" => 11 bytes.

Wouldn't this function return 10 in the case of /memory@0?
Are you saying that it should count the \0 at the end too? it's not
doing that, AFAICS.

> + *
> + * Returns the string length of the node's full path.
> + */

Maybe it's me (by analogy with strlen()), but I would expect "string
length" to not include the terminating \0. I suggest renaming the
function to something like get_node_path_size() and do s/length/size/ in
the comment above if it's supposed to count the terminating \0.

> +static int get_node_pathlen(struct device_node *dn)
> +{
> +	int len = 0;
> +
> +	if (!dn)
> +		return 0;
> +
> +	while (dn) {
> +		len += strlen(dn->full_name) + 1;
> +		dn = dn->parent;
> +	}
> +
> +	return len + 1;
> +}
> +
> +/**
> + * 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)
> +{
> +	struct device_node *dn;
> +	int len, idx, nlen;
> +	char *path = NULL;
> +	char end_char;
> +
> +	if (!node)
> +		goto err;
> +
> +	/*
> +	 * Get the path length first and use it to iteratively build the path
> +	 * from node to root.
> +	 */
> +	len = get_node_pathlen(node);
> +
> +	/* Allocate memory for node path */
> +	path = kzalloc(ALIGN(len, 8), GFP_KERNEL);
> +	if (!path)
> +		goto err;
> +
> +	/*
> +	 * Iteratively update path from node to root by decrementing
> +	 * index appropriately.
> +	 *
> +	 * Also, add %NUL at the end of node & '/' at the end of all its
> +	 * parent nodes.
> +	 */
> +	dn = node;
> +	path[0] = '/';
> +	idx = len - 1;

Here, idx is pointing to the supposed '/' at the end of the node
path ...

> +	end_char = '\0';
> +	while (dn->parent) {
> +		path[--idx] = end_char;

.. and in the first ireation, this is writing '\0' at a place which will be
overwritten by the memcpy() below with the last character of
dn->full_name. You need to start idx with len, not len - 1.

> +		end_char = '/';
> +
> +		nlen = strlen(dn->full_name);
> +		idx -= nlen;
> +		memcpy(path + idx, dn->full_name, nlen);
> +
> +		dn = dn->parent;
> +	}
> +
> +	return path;
> +err:
> +	kfree(path);
> +	return NULL;
> +}

--
Thiago Jung Bauermann
IBM Linux Technology Center