Re: [PATCH] arm64: head: avoid over-mapping in map_memory

From: Will Deacon <will@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, anshuman.khandual@arm.com,
	ard.biesheuvel@linaro.org, catalin.marinas@arm.com,
	steve.capper@arm.com
Subject: Re: [PATCH] arm64: head: avoid over-mapping in map_memory
Date: Wed, 11 Aug 2021 10:44:58 +0100	[thread overview]
Message-ID: <20210811094457.GA4426@willie-the-truck> (raw)
In-Reply-To: <20210810152756.23903-1-mark.rutland@arm.com>

Hi Mark,

On Tue, Aug 10, 2021 at 04:27:56PM +0100, Mark Rutland wrote:
> The `compute_indices` and `populate_entries` macros operate on inclusive
> bounds, and thus the `map_memory` macro which uses them also operates
> on inclusive bounds.
> 
> We pass `_end` and `_idmap_text_end` to `map_memory`, but these are
> exclusive bounds, and if one of these is sufficiently aligned (as a
> result of kernel configuration, physical placement, and KASLR), then:
> 
> * In `compute_indices`, the computed `iend` will be in the page/block *after*
>   the final byte of the intended mapping.
> 
> * In `populate_entries`, an unnecessary entry will be created at the end
>   of each level of table. At the leaf level, this entry will map up to
>   SWAPPER_BLOCK_SIZE bytes of physical addresses that we did not intend
>   to map.
> 
> As we may map up to SWAPPER_BLOCK_SIZE bytes more than intended, we may
> violate the boot protocol and map physical address past the 2MiB-aligned
> end address we are permitted to map. As we map these with Normal memory
> attributes, this may result in further problems depending on what these
> physical addresses correspond to.
> 
> Fix this by subtracting one from the end address in both cases, such
> that we always use inclusive bounds. For clarity, comments are updated
> to more clearly document that the macros expect inclusive bounds.

I think the comments are already clear for populate_entries and map_memory,
so just adding something to compute_indices should suffice. As-is, your
patch leaves populate_entries inconsistent with the others.

That aside, I don't think any amount of comments will help because it's
just plain weird for vend to be inclusive for map_memory, no? Given that
both (all) callers got this wrong, I'd be inclined to say that map_memory
should take an exclusive 'vend' as you'd normally expect and it can adjust
it before using the helper macros (which should perhaps have 'inclusive' in
their names).

Although we currently preserve 'vend' in map_memory, that doesn't appear to
be necessary so we can just move it to the list of clobbers in the comment
nobody reads.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel