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

From: Jan Beulich <jbeulich@suse.com>
To: Roger Pau Monne <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] x86/mm: fix detection of last L1 entry in modify_xen_mappings_lite()
Date: Mon, 11 Mar 2024 12:29:30 +0100	[thread overview]
Message-ID: <ac38c8bf-56c2-49f8-98fe-1563803a9c14@suse.com> (raw)
In-Reply-To: <20240311105416.4556-1-roger.pau@citrix.com>

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

Jan