Re: double free in alternatives/retpoline

Re: double free in alternatives/retpoline

[Linus Torvalds]

[ Adding Mike Rapoport ]

On Wed, 18 Jun 2025 at 19:08, Dave Airlie <airlied@gmail.com> wrote:
>
> I've just tried to boot Linux master with KASAN enabled on a laptop here, and it showing a slab UAF for apply_retpolines.
>
> I haven't had a chance to bisect yet, and unfortunately I only have a photo of the oops.

Hmm.

I think it's due to commit a82b26451de1 ("x86/its: explicitly manage
permissions for ITS pages").

Maybe I'm mis-reading it entirely, but I think that "its_fini_core()"
thing is entirely bogus. It does that

        kfree(its_pages.pages);

but as far as I can tell, that thing is happily used later by module
initialization.

Freeing the pages that have been used and marked ROX sounds like it
should be fine, but I think it should also do

        its_pages.pages = NULL;
        its_pages->num = 0;

so that any subsequent user that comes along due to modules or
whatever and does __its_alloc() will DTRT wrt the realloc().

But I might be completely barking up the wrong tree and mis-reading
things entirely. PeterZ? Mike?

             Linus

Re: double free in alternatives/retpoline

[Dave Airlie]

On Thu, 19 Jun 2025 at 12:33, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> [ Adding Mike Rapoport ]
>
> On Wed, 18 Jun 2025 at 19:08, Dave Airlie <airlied@gmail.com> wrote:
> >
> > I've just tried to boot Linux master with KASAN enabled on a laptop here, and it showing a slab UAF for apply_retpolines.
> >
> > I haven't had a chance to bisect yet, and unfortunately I only have a photo of the oops.
>
> Hmm.
>
> I think it's due to commit a82b26451de1 ("x86/its: explicitly manage
> permissions for ITS pages").

It's definitely ITS related, turning off CONFIG_MITIGATIONS_ITS lets
two test machines I have boot.

Dave.
>
> Maybe I'm mis-reading it entirely, but I think that "its_fini_core()"
> thing is entirely bogus. It does that
>
>         kfree(its_pages.pages);
>
> but as far as I can tell, that thing is happily used later by module
> initialization.
>
> Freeing the pages that have been used and marked ROX sounds like it
> should be fine, but I think it should also do
>
>         its_pages.pages = NULL;
>         its_pages->num = 0;
>
> so that any subsequent user that comes along due to modules or
> whatever and does __its_alloc() will DTRT wrt the realloc().
>
> But I might be completely barking up the wrong tree and mis-reading
> things entirely. PeterZ? Mike?
>
>              Linus

Re: double free in alternatives/retpoline

[Dave Airlie]

On Thu, 19 Jun 2025 at 12:33, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> [ Adding Mike Rapoport ]
>
> On Wed, 18 Jun 2025 at 19:08, Dave Airlie <airlied@gmail.com> wrote:
> >
> > I've just tried to boot Linux master with KASAN enabled on a laptop here, and it showing a slab UAF for apply_retpolines.
> >
> > I haven't had a chance to bisect yet, and unfortunately I only have a photo of the oops.
>
> Hmm.
>
> I think it's due to commit a82b26451de1 ("x86/its: explicitly manage
> permissions for ITS pages").
>
> Maybe I'm mis-reading it entirely, but I think that "its_fini_core()"
> thing is entirely bogus. It does that
>
>         kfree(its_pages.pages);
>
> but as far as I can tell, that thing is happily used later by module
> initialization.
>
> Freeing the pages that have been used and marked ROX sounds like it
> should be fine, but I think it should also do
>
>         its_pages.pages = NULL;
>         its_pages->num = 0;
>
> so that any subsequent user that comes along due to modules or
> whatever and does __its_alloc() will DTRT wrt the realloc().
>
> But I might be completely barking up the wrong tree and mis-reading
> things entirely. PeterZ? Mike?

I wonder if the module code also needs the same treatment,

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 6455f7f751b3..4653881a4ab3 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -182,6 +182,7 @@ static void its_fini_core(void)
     if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
         its_pages_protect(&its_pages);
     kfree(its_pages.pages);
+    its_pages.pages = NULL;
 }

 #ifdef CONFIG_MODULES
(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? y
@@ -220,6 +221,8 @@ void its_free_mod(struct module *mod)
         execmem_free(page);
     }
     kfree(mod->arch.its_pages.pages);
+    mod->arch.its_pages.pages = NULL;
+    mod->arch.its_pages.num = 0;
 }
 #endif /* CONFIG_MODULES */

boots for me, but I've no idea what is required or sufficient.

Dave.
>
>              Linus

Re: double free in alternatives/retpoline

[Mike Rapoport]

Hi Dave,

On Thu, Jun 19, 2025 at 01:31:19PM +1000, Dave Airlie wrote:
> On Thu, 19 Jun 2025 at 12:33, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Wed, 18 Jun 2025 at 19:08, Dave Airlie <airlied@gmail.com> wrote:
> > >
> > > I've just tried to boot Linux master with KASAN enabled on a laptop here, and it showing a slab UAF for apply_retpolines.

Does oops happen in core code or when a module is loaded?
Can you share the kernel config?

> > > I haven't had a chance to bisect yet, and unfortunately I only have a photo of the oops.

Could still be useful :)

> > Hmm.
> >
> > I think it's due to commit a82b26451de1 ("x86/its: explicitly manage
> > permissions for ITS pages").
> >
> > Maybe I'm mis-reading it entirely, but I think that "its_fini_core()"
> > thing is entirely bogus. It does that
> >
> >         kfree(its_pages.pages);
> >
> > but as far as I can tell, that thing is happily used later by module
> > initialization.
> >
> > Freeing the pages that have been used and marked ROX sounds like it
> > should be fine, but I think it should also do
> >
> >         its_pages.pages = NULL;
> >         its_pages->num = 0;
> >
> > so that any subsequent user that comes along due to modules or
> > whatever and does __its_alloc() will DTRT wrt the realloc().

its_fini_core() is called after all pages for the core kernel are
allocated, so there should be no reallocs that use its_pages.pages after
that. 

Modules use a different array stored in the module structure itself and
that array is freed together with the reset of the module when the module
is removed.

Obviously I overlooked something because there's that UAF splat, but it's
not that module initialization reuses the same its_pages.

> > But I might be completely barking up the wrong tree and mis-reading
> > things entirely. PeterZ? Mike?
> 
> I wonder if the module code also needs the same treatment,
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 6455f7f751b3..4653881a4ab3 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -182,6 +182,7 @@ static void its_fini_core(void)
>      if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
>          its_pages_protect(&its_pages);
>      kfree(its_pages.pages);
> +    its_pages.pages = NULL;
>  }
> 
>  #ifdef CONFIG_MODULES
> (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? y
> @@ -220,6 +221,8 @@ void its_free_mod(struct module *mod)
>          execmem_free(page);
>      }
>      kfree(mod->arch.its_pages.pages);
> +    mod->arch.its_pages.pages = NULL;
> +    mod->arch.its_pages.num = 0;
>  }
>  #endif /* CONFIG_MODULES */
> 
> boots for me, but I've no idea what is required or sufficient.

This surely won't hurt, but I'd like to understand first why there was a
UAF spat at the first place.
 
> Dave.
> >
> >              Linus

-- 
Sincerely yours,
Mike.

Re: double free in alternatives/retpoline

[Mike Rapoport]

On Thu, Jun 19, 2025 at 01:31:19PM +1000, Dave Airlie wrote:
> On Thu, 19 Jun 2025 at 12:33, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > [ Adding Mike Rapoport ]
> >
> > On Wed, 18 Jun 2025 at 19:08, Dave Airlie <airlied@gmail.com> wrote:
> > >
> > > I've just tried to boot Linux master with KASAN enabled on a laptop here, and it showing a slab UAF for apply_retpolines.
> > >
> > > I haven't had a chance to bisect yet, and unfortunately I only have a photo of the oops.
> >
> > Hmm.
> >
> > I think it's due to commit a82b26451de1 ("x86/its: explicitly manage
> > permissions for ITS pages").
> >
> > Maybe I'm mis-reading it entirely, but I think that "its_fini_core()"
> > thing is entirely bogus. It does that
> >
> >         kfree(its_pages.pages);
> >
> > but as far as I can tell, that thing is happily used later by module
> > initialization.
> >
> > Freeing the pages that have been used and marked ROX sounds like it
> > should be fine, but I think it should also do
> >
> >         its_pages.pages = NULL;
> >         its_pages->num = 0;
> >
> > so that any subsequent user that comes along due to modules or
> > whatever and does __its_alloc() will DTRT wrt the realloc().
> >
> > But I might be completely barking up the wrong tree and mis-reading
> > things entirely. PeterZ? Mike?
> 
> I wonder if the module code also needs the same treatment,

Looking more closely, there is a typo in its_alloc(), it uses CONFIG_MODULE
to choose an its_array to reallocate while it should have been using
CONFIG_MODULES.

Lukas Bulwahn sent a fix for that:

https://lore.kernel.org/all/20250616100432.22941-1-lukas.bulwahn@redhat.com
 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 6455f7f751b3..4653881a4ab3 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -182,6 +182,7 @@ static void its_fini_core(void)
>      if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
>          its_pages_protect(&its_pages);
>      kfree(its_pages.pages);
> +    its_pages.pages = NULL;
>  }
> 
>  #ifdef CONFIG_MODULES
> (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? y
> @@ -220,6 +221,8 @@ void its_free_mod(struct module *mod)
>          execmem_free(page);
>      }
>      kfree(mod->arch.its_pages.pages);
> +    mod->arch.its_pages.pages = NULL;
> +    mod->arch.its_pages.num = 0;
>  }
>  #endif /* CONFIG_MODULES */
> 
> boots for me, but I've no idea what is required or sufficient.
> 
> Dave.
> >
> >              Linus

-- 
Sincerely yours,
Mike.

Re: double free in alternatives/retpoline

[Peter Zijlstra]

On Thu, Jun 19, 2025 at 11:06:53AM +0300, Mike Rapoport wrote:

> Looking more closely, there is a typo in its_alloc(), it uses CONFIG_MODULE
> to choose an its_array to reallocate while it should have been using
> CONFIG_MODULES.
> 
> Lukas Bulwahn sent a fix for that:
> 
> https://lore.kernel.org/all/20250616100432.22941-1-lukas.bulwahn@redhat.com

Indeed. That fix is currently sitting in tip/x86/urgent and should make
its way to Linus soon.