linux-kernel - Re: [PATCH] of: Check for overlap in reserved memory regions

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]

Date:	Mon, 14 Sep 2015 18:16:31 -0700
From:	Mitchel Humpherys <mitchelh@...eaurora.org>
To:	Rob Herring <robh@...nel.org>
Cc:	Rob Herring <robh+dt@...nel.org>,
	Frank Rowand <frowand.list@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	"devicetree\@vger.kernel.org" <devicetree@...r.kernel.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] of: Check for overlap in reserved memory regions

On Mon, Sep 14 2015 at 02:21:04 PM, Rob Herring <robh@...nel.org> wrote:
> On Fri, Sep 11, 2015 at 12:31 PM, Mitchel Humpherys
> <mitchelh@...eaurora.org> wrote:
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug.  These bugs might go undetected as
>> long as the contested region isn't used simultaneously by multiple
>> software agents, which makes such bugs hard to debug.  Fix this by
>> printing a scary warning during boot if overlap is detected.

[...]

>> +
>> +static void __init __rmem_check_for_overlap(void)
>> +{
>> +       int i;
>> +
>> +       if (reserved_mem_count < 2)
>> +               return;
>> +
>> +       memcpy(sorted_reserved_mem, reserved_mem, sizeof(sorted_reserved_mem));
>> +       sort(sorted_reserved_mem, reserved_mem_count,
>> +            sizeof(sorted_reserved_mem[0]), __rmem_cmp, NULL);
>
> Why not just sort reserved_mem?

Yeah, I considered that but wasn't sure if it would break things in the
few places where the ordering of reserved_mem matters (like
__find_rmem).  Looking closer I think we're safe to sort reserved_mem
array directly to avoid the memcpy.  Will update in v2.

>
>> +       for (i = 0; i < reserved_mem_count - 1; i++) {
>> +               struct reserved_mem *this, *next;
>> +
>> +               this = &sorted_reserved_mem[i];
>> +               next = &sorted_reserved_mem[i + 1];
>> +               if (!(this->base && next->base))
>> +                       continue;
>> +               if (this->base + this->size > next->base) {
>> +                       phys_addr_t this_end, next_end;
>> +
>> +                       this_end = this->base + this->size;
>> +                       next_end = next->base + next->size;
>> +                       WARN(1, "Reserved mem: OVERLAP DETECTED!\n");
>> +                       pr_err("%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
>
> This seems overly verbose having both WARN and pr_err. I'd combine
> these. I don't think the stack trace from a WARN is too useful here
> given it is the DT file that users will need to go look at.

Yeah the reason for the WARN is to make it harder for this to slip by,
and I thought a BUG might be too heavy-handed.  I should be able to
squeeze the content of the pr_err into the WARN in v2.


-Mitch

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/