Re: [PATCH] x86/mm: fix detection of last L1 entry in modify_xen_mappings_lite()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/03/2024 11:29 am, Jan Beulich wrote:
> On 11.03.2024 11:54, Roger Pau Monne wrote:
>> The current logic to detect when to switch to the next L1 table is 
>> incorrectly
>> using l2_table_offset() in order to notice when the last entry on the current
>> L1 table has been reached.
>>
>> It should instead use l1_table_offset() to check whether the index has 
>> wrapped
>> to point to the first entry, and so the next L1 table should be used.
>>
>> Fixes: 8676092a0f16 ('x86/livepatch: Fix livepatch application when CET is 
>> active')
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> ---
>> This fixes the osstest livepatch related crash, we have been lucky so far 
>> that
>> the .text section didn't seem to have hit this.
> About half a megabyte more to go until .text could run into such an issue,
> I guess, just considering the core Xen image. Patches are presumably not
> large enough to stand a sufficient risk of hitting the issue.
>
> I think there's another latent problem though, related to this part of the
> comment ahead of the function:
>
>  * It is the callers responsibility to not pass s or e in the middle of
>  * superpages if changing the permission on the whole superpage is going to be
>  * a problem.
>
> This only suggests that for a pointer into the middle of a superpage the
> effect may be wider than intended. But with s misaligned modulo 2Mb the
> superpage part of the loop would keep v misaligned, and if the 2nd 2Mb
> range wasn't a superpage, part of the range wouldn't be touched at all.
> Right now with .text always 2Mb-aligned (and with there not being a
> superpage mapping across _srodata) there's no issue as long as superpages
> aren't used in patch loading. Yet recall that .text used to be only 1Mb
> aligned in older Xen versions, and this fact isn't entirely set in stone
> when !XEN_ALIGN_2M.

That comment was added at your request.

The start address is always going to a linker symbol in the main image,
or something allocated with MAP_SMALL_PAGES.

Xen's .text strictly is 2M aligned.  The boot time pagetable handling
doesn't otherwise.

But either way.  The two options are to either ASSERT() that v is 2M
aligned when finding PSE, or to realign it on each iteration.  I'd
prefer to go with the assert on the basis that I don't expect this
property to be violated in practice.

I also note that forcing page alignment on e is useless.  All it does is
force the caller to do pointless work.

~Andrew