RE: [Patch v3 01/14] x86/ioremap: Fix page aligned size calculation in __ioremap_caller()

[Michael Kelley (LINUX)]

From: Borislav Petkov <bp@xxxxxxxxx> Sent: Monday, November 21, 2022 5:33 AM
> 
> On Wed, Nov 16, 2022 at 10:41:24AM -0800, Michael Kelley wrote:
> > Current code re-calculates the size after aligning the starting and
> > ending physical addresses on a page boundary. But the re-calculation
> > also embeds the masking of high order bits that exceed the size of
> > the physical address space (via PHYSICAL_PAGE_MASK). If the masking
> > removes any high order bits, the size calculation results in a huge
> > value that is likely to immediately fail.
> >
> > Fix this by re-calculating the page-aligned size first. Then mask any
> > high order bits using PHYSICAL_PAGE_MASK.
> >
> > Signed-off-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/mm/ioremap.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > index 78c5bc6..6453fba 100644
> > --- a/arch/x86/mm/ioremap.c
> > +++ b/arch/x86/mm/ioremap.c
> > @@ -217,9 +217,15 @@ static void __ioremap_check_mem(resource_size_t addr,
> unsigned long size,
> >  	 * Mappings have to be page-aligned
> >  	 */
> >  	offset = phys_addr & ~PAGE_MASK;
> > -	phys_addr &= PHYSICAL_PAGE_MASK;
> > +	phys_addr &= PAGE_MASK;
> >  	size = PAGE_ALIGN(last_addr+1) - phys_addr;
> >
> > +	/*
> > +	 * Mask out any bits not part of the actual physical
> > +	 * address, like memory encryption bits.
> > +	 */
> > +	phys_addr &= PHYSICAL_PAGE_MASK;
> > +
> >  	retval = memtype_reserve(phys_addr, (u64)phys_addr + size,
> >  						pcm, &new_pcm);
> >  	if (retval) {
> > --
> 
> This looks like a fix to me that needs to go to independently to stable.
> And it would need a Fixes tag.
> 
> /me does some git archeology...
> 
> I guess this one:
> 
> ffa71f33a820 ("x86, ioremap: Fix incorrect physical address handling in PAE mode")
> 
> should be old enough so that it goes to all relevant stable kernels...
> 
> Hmm?
> 

As discussed in a parallel thread [1], the incorrect code here doesn't have
any real impact in already released Linux kernels.  It only affects the
transition that my patch series implements to change the way vTOM
is handled.

I don't know what the tradeoffs are for backporting a fix that doesn't solve
a real problem vs. just letting it be.  Every backport carries some overhead
in the process and there's always a non-zero risk of breaking something.
I've leaned away from adding the "Fixes:" tag in such cases.  But if it's better
to go ahead and add the "Fixes:" tag for what's only a theoretical problem,
I'm OK with doing so.

Michael

[1] https://lkml.org/lkml/2022/11/11/1348