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

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Aug 11, 2021 at 10:44:58AM +0100, Will Deacon wrote:
> 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.

Sorry, I'd left that in a separate patch that also renamed index/eindex
to istart/iend for consistency with compute_indices. I should have
folded that change in.

> 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?

I guess; I have no strong feeling either way.

> 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).

Sure, I'm also happy with that.

> 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.

Works for me.

Thanks,
Mark.

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