Re: [PATCH v5 03/16] x86/alternatives: Disable LASS when patching kernel alternatives

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Oct 28, 2024 at 10:49:07AM -0700, Dave Hansen wrote:
> On 10/28/24 09:07, Alexander Shishkin wrote:
> >  static void text_poke_memcpy(void *dst, const void *src, size_t len)
> >  {
> > -	memcpy(dst, src, len);
> > +	lass_stac();
> > +	__inline_memcpy(dst, src, len);
> > +	lass_clac();
> >  }
> >  
> >  static void text_poke_memset(void *dst, const void *src, size_t len)
> >  {
> >  	int c = *(const int *)src;
> >  
> > -	memset(dst, c, len);
> > +	lass_stac();
> > +	__inline_memset(dst, c, len);
> > +	lass_clac();
> >  }
> 
> These are the _only_ users of lass_stac/clac() or the new inlines.

For now; I have vague memories of running into trouble with compilers
doing random things with memcpy before, and having these inline versions
gives us more control.

One of the cases I remember running into was KASAN, where a compiler is
SUPPOSED to issue __asan_memcpy calls instead of the regular memcpy
calls, except they weren't all doing that, with the end result that our
regular memcpy implementation grew instrumentation to deal with that.

That got sorted -- by deprecating / breaking all those non-conformant
compilers. But still, I think it would be good to have the option to
force a simple inline memcpy when needed.

> First of all, I totally agree that the _existing_ strict objtool
> behavior around STAC/CLAC is a good idea.
> 
> But text poking really is special and the context is highly unlikely to
> result in bugs or exploits.  My first instinct here would have been to
> tell objtool that the text poking code is OK and to relax objtool's
> STAC/CLAC paranoia here.
> 
> Looking at objtool, I can see how important it is to keep the STAC/CLAC
> code as dirt simple and foolproof as possible.  I don't see an obvious
> way to except the text poking code without adding at least some complexity.
> 
> Basically what I'm asking for is if the goal is to keep objtool simple,
> please *SAY* that.  Because on the surface this doesn't look like a good
> idea.

There is, you can add it to uaccess_safe_builtin[], but I'm not sure we
want to blanked accept memcpy() -- or perhaps that is what you're
saying.

Anyway, looking at this, I see we grew rep_{movs,stos}_alternative, as
used in copy_user_generic() and __clear_user(). Which are all somewhat
similar.