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....@citrix.com>
>> ---
>> 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
