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

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:

quoted

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