linux-hardening - Re: [PATCH v4 1/2] mm/memblock: Add "reserve

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240611183643.0327ef5b@gandalf.local.home>
Date: Tue, 11 Jun 2024 18:36:43 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Guenter Roeck <linux@...ck-us.net>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, Masami
 Hiramatsu <mhiramat@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Andrew Morton
 <akpm@...ux-foundation.org>, "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 Vlastimil Babka <vbabka@...e.cz>, Lorenzo Stoakes <lstoakes@...il.com>,
 linux-mm@...ck.org, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
 <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
 <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin"
 <hpa@...or.com>, Peter Zijlstra <peterz@...radead.org>, Kees Cook
 <keescook@...omium.org>, Tony Luck <tony.luck@...el.com>, "Guilherme G.
 Piccoli" <gpiccoli@...lia.com>, linux-hardening@...r.kernel.org, Ross
 Zwisler <zwisler@...gle.com>, wklin@...gle.com, Vineeth Remanan Pillai
 <vineeth@...byteword.org>, Joel Fernandes <joel@...lfernandes.org>,
 Suleiman Souhlal <suleiman@...gle.com>, Linus Torvalds
 <torvalds@...uxfoundation.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>, Ard Biesheuvel <ardb@...nel.org>, Mike
 Rapoport <rppt@...nel.org>
Subject: Re: [PATCH v4 1/2] mm/memblock: Add "reserve_mem" to reserved named
 memory at boot up

On Tue, 11 Jun 2024 15:26:05 -0700
Guenter Roeck <linux@...ck-us.net> wrote:

> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index d09136e040d3..044ddce8f085 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -2244,6 +2244,121 @@ void __init memblock_free_all(void)
> >   	totalram_pages_add(pages);
> >   }
> >   
> > +/* Keep a table to reserve named memory */
> > +#define RESERVE_MEM_MAX_ENTRIES		8
> > +#define RESERVE_MEM_NAME_SIZE		16
> > +struct reserve_mem_table {
> > +	char			name[RESERVE_MEM_NAME_SIZE];
> > +	phys_addr_t		start;
> > +	phys_addr_t		size;
> > +};
> > +static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
> > +static int reserved_mem_count;
> > +
> > +/* Add wildcard region with a lookup name */
> > +static int __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> > +				   const char *name)
> > +{
> > +	struct reserve_mem_table *map;
> > +
> > +	if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
> > +		return -EINVAL;
> > +  
> 
> I know I am picky, but name is never NULL, and strlen(name) is guaranteed to be > 0.
> Personally I'd suggest to check for strlen(name) >= RESERVE_MEM_NAME_SIZE together
> with !strlen(name) and drop the duplicate checks here (and, as side effect, avoid
> the pointless memory allocation if the name is invalid).

Yeah, it's now checked before hand. I'll remove it for v5.

> 
> > +	if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
> > +		return -1;
> > +
> > +	map = &reserved_mem_table[reserved_mem_count++];
> > +	map->start = start;
> > +	map->size = size;
> > +	strscpy(map->name, name);
> > +	return 0;
> > +}
> > +

> > +/*
> > + * Parse reserve_mem=nn:align:name
> > + */
> > +static int __init reserve_mem(char *p)
> > +{
> > +	phys_addr_t start, size, align;
> > +	char *name;
> > +	char *oldp;
> > +	int err;
> > +
> > +	if (!p)
> > +		return -EINVAL;
> > +
> > +	oldp = p;
> > +	size = memparse(p, &p);
> > +	if (!size || p == oldp)
> > +		return -EINVAL;
> > +
> > +	if (*p != ':')
> > +		return -EINVAL;
> > +
> > +	align = memparse(p+1, &p);
> > +	if (*p != ':')
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * memblock_phys_alloc() doesn't like a zero size align,
> > +	 * but it is OK for this command to have it.
> > +	 */
> > +	if (align <= SMP_CACHE_BYTES)  
> 
> Any reason for using <= instead of < ?

Nope. Not sure why I did that. :-/

I'll fix that too.

Thanks,

-- Steve

> 
> Guenter
> 
> > +		align = SMP_CACHE_BYTES;
> > +
> > +	name = p + 1;
> > +	if (!strlen(name))
> > +		return -EINVAL;
> > +
> > +	/* Make sure that name has text */
> > +	for (p = name; *p; p++) {
> > +		if (!isspace(*p))
> > +			break;
> > +	}
> > +	if (!*p)
> > +		return -EINVAL;
> > +
> > +	start = memblock_phys_alloc(size, align);
> > +	if (!start)
> > +		return -ENOMEM;
> > +
> > +	err = reserved_mem_add(start, size, name);
> > +	if (err) {
> > +		memblock_phys_free(start, size);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +__setup("reserve_mem=", reserve_mem);
> > +
> >   #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> >   static const char * const flagname[] = {
> >   	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",