linux-kernel - Re: [PATCH v2 1/4] arm64: export memblock

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <891c3e5a-9760-1353-4a2c-0900082dd245@oracle.com>
Date:   Tue, 19 Jun 2018 10:22:46 -0500
From:   Dave Kleikamp <dave.kleikamp@...cle.com>
To:     James Morse <james.morse@....com>,
        AKASHI Takahiro <takahiro.akashi@...aro.org>,
        catalin.marinas@....com, will.deacon@....com,
        akpm@...ux-foundation.org, ard.biesheuvel@...aro.org
Cc:     mark.rutland@....com, lorenzo.pieralisi@....com,
        graeme.gregory@...aro.org, al.stone@...aro.org,
        bhsharma@...hat.com, tbaicar@...eaurora.org,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
        hanjun.guo@...aro.org, sudeep.holla@....com, dyoung@...hat.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/4] arm64: export memblock_reserve()d regions via
 /proc/iomem

On 06/19/2018 10:00 AM, James Morse wrote:
> Hi Dave,
> 
> On 19/06/18 14:37, Dave Kleikamp wrote:
>> On 06/19/2018 01:44 AM, AKASHI Takahiro wrote:
>>> +static int __init reserve_memblock_reserved_regions(void)
>>> +{
>>> +	phys_addr_t start, end, roundup_end = 0;
>>> +	struct resource *mem, *res;
>>> +	u64 i;
>>> +
>>> +	for_each_reserved_mem_region(i, &start, &end) {
>>> +		if (end <= roundup_end)
>>> +			continue; /* done already */
>>> +
>>> +		start = __pfn_to_phys(PFN_DOWN(start));
>>> +		end = __pfn_to_phys(PFN_UP(end)) - 1;
>>> +		roundup_end = end;
>>> +
>>> +		res = kzalloc(sizeof(*res), GFP_ATOMIC);
>>> +		if (WARN_ON(!res))
>>> +			return -ENOMEM;
>>> +		res->start = start;
>>> +		res->end = end;
>>> +		res->name  = "reserved";
>>> +		res->flags = IORESOURCE_MEM;
>>> +
>>> +		mem = request_resource_conflict(&iomem_resource, res);
>>> +		/*
>>> +		 * We expected memblock_reserve() regions to conflict with
>>> +		 * memory created by request_standard_resources().
>>> +		 */
>>> +		if (WARN_ON_ONCE(!mem))
>>> +			continue;
>>> +		kfree(res);
>>
>> Why is kfree() after the conditional continue? This is a memory leak.
> 
> request_resource_conflict() inserts res into the iomem_resource tree, or returns
> the conflict if there is already something at this address.
> 
> We expect something at this address: a 'System RAM' section added by
> request_resource(). This code wants the conflicting 'System RAM' entry so that
> reserve_region_with_split() can fill in the gaps below it with 'reserved'. See
> the commit-message for an example.
> 
> If there was no conflict, it means the memory map doesn't look like we expect,
> we can't pass NULL to reserve_region_with_split(), and we just inserted the
> 'reserved' at the top level. Freeing res at this point would be a use-after-free
> hanging from /proc/iomem.
> This code generates a WARN_ON_ONCE() and leaves the 'reserved' description where
> it is.

Okay. I get it.

> Trying to cleanup here is pointless, we can remove the resource entry and free
> it ... but we still want to report it as reserved, which is what just happened,
> just not quite how we we wanted it.
> 
> If you ever see this warning, it means some RAM stopped being nomap between
> request_resources() and reserve_memblock_reserved_regions(). I can't find any
> case where that ever happens.
> 
> 
> If all that makes sense: how can I improve the comment above the WARN_ON_ONCE()
> to make it clearer?

I guess something like you described above.

/*
 * We expect a 'System RAM' section at this address. In the unexpected
 * case where one is not found, request_resource_conflict() will insert
 * res into the iomem_resource tree.
 */

Do you think this is clearer?

> 
> 
> Thanks,
> 
> James
> 
> 
>>> +
>>> +		reserve_region_with_split(mem, start, end, "reserved");
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +arch_initcall(reserve_memblock_reserved_regions);
>>> +
>>>  u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
>>>  
>>>  void __init setup_arch(char **cmdline_p)
>>>
>