| Message ID | 20240311105416.4556-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mm: fix detection of last L1 entry in modify_xen_mappings_lite() | expand |
On 11/03/2024 10:54 am, 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> Reviewed-by: Andrew Cooper <andrew.cooper3@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. I'm very surprised too.
On 11/03/2024 11:01, Andrew Cooper wrote: > On 11/03/2024 10:54 am, 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> > > Reviewed-by: Andrew Cooper <andrew.cooper3@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. > > I'm very surprised too. > The two stages for software debugging: 1. I really don't see how it can possibly fail. 2. I really don't see how it could have possibly worked.
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
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@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
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 2aff6d4b5338..0c6658298de2 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5959,7 +5959,7 @@ void init_or_livepatch modify_xen_mappings_lite( v += 1UL << L1_PAGETABLE_SHIFT; - if ( l2_table_offset(v) == 0 ) + if ( l1_table_offset(v) == 0 ) break; }
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. --- xen/arch/x86/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)