Re: [PATCH v5 4/8] lkdtm/x86_64: Add test to hijack a patch mapping

["Christopher M. Riedl" <cmr@linux.ibm.com>]

On Thu Aug 5, 2021 at 4:09 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > A previous commit implemented an LKDTM test on powerpc to exploit the
> > temporary mapping established when patching code with STRICT_KERNEL_RWX
> > enabled. Extend the test to work on x86_64 as well.
> > 
> > Signed-off-by: Christopher M. Riedl <cmr@linux.ibm.com>
> > ---
> >   drivers/misc/lkdtm/perms.c | 26 ++++++++++++++++++++++----
> >   1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > index 39e7456852229..41e87e5f9cc86 100644
> > --- a/drivers/misc/lkdtm/perms.c
> > +++ b/drivers/misc/lkdtm/perms.c
> > @@ -224,7 +224,7 @@ void lkdtm_ACCESS_NULL(void)
> >   }
> >   
> >   #if (IS_BUILTIN(CONFIG_LKDTM) && defined(CONFIG_STRICT_KERNEL_RWX) && \
> > -	defined(CONFIG_PPC))
> > +	(defined(CONFIG_PPC) || defined(CONFIG_X86_64)))
> >   /*
> >    * This is just a dummy location to patch-over.
> >    */
> > @@ -233,12 +233,25 @@ static void patching_target(void)
> >   	return;
> >   }
> >   
> > -#include <asm/code-patching.h>
> >   const u32 *patch_site = (const u32 *)&patching_target;
> >   
> > +#ifdef CONFIG_PPC
> > +#include <asm/code-patching.h>
> > +#endif
> > +
> > +#ifdef CONFIG_X86_64
> > +#include <asm/text-patching.h>
> > +#endif
> > +
> >   static inline int lkdtm_do_patch(u32 data)
> >   {
> > +#ifdef CONFIG_PPC
> >   	return patch_instruction((u32 *)patch_site, ppc_inst(data));
> > +#endif
> > +#ifdef CONFIG_X86_64
> > +	text_poke((void *)patch_site, &data, sizeof(u32));
> > +	return 0;
> > +#endif
> >   }
> >   
> >   static inline u32 lkdtm_read_patch_site(void)
> > @@ -249,11 +262,16 @@ static inline u32 lkdtm_read_patch_site(void)
> >   /* Returns True if the write succeeds */
> >   static inline bool lkdtm_try_write(u32 data, u32 *addr)
> >   {
> > +#ifdef CONFIG_PPC
> >   	__put_kernel_nofault(addr, &data, u32, err);
> >   	return true;
> >   
> >   err:
> >   	return false;
> > +#endif
> > +#ifdef CONFIG_X86_64
> > +	return !__put_user(data, addr);
> > +#endif
> >   }
> >   
> >   static int lkdtm_patching_cpu(void *data)
> > @@ -346,8 +364,8 @@ void lkdtm_HIJACK_PATCH(void)
> >   
> >   void lkdtm_HIJACK_PATCH(void)
> >   {
> > -	if (!IS_ENABLED(CONFIG_PPC))
> > -		pr_err("XFAIL: this test only runs on powerpc\n");
> > +	if (!IS_ENABLED(CONFIG_PPC) && !IS_ENABLED(CONFIG_X86_64))
> > +		pr_err("XFAIL: this test only runs on powerpc and x86_64\n");
> >   	if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> >   		pr_err("XFAIL: this test requires CONFIG_STRICT_KERNEL_RWX\n");
> >   	if (!IS_BUILTIN(CONFIG_LKDTM))
> > 
>
> Instead of spreading arch specific stuff into LKDTM, wouldn't it make
> sence to define common a
> common API ? Because the day another arch like arm64 implements it own
> approach, do we add specific
> functions again and again into LKDTM ?

Hmm a common patch/poke kernel API is probably out of scope for this
series? I do agree though - since you suggested splitting the series
maybe that's something I can add along with the LKDTM patches.

>
> Also, I find it odd to define tests only when they can succeed. For
> other tests like
> ACCESS_USERSPACE, they are there all the time, regardless of whether we
> have selected
> CONFIG_PPC_KUAP or not. I think it should be the same here, have it all
> there time, if
> CONFIG_STRICT_KERNEL_RWX is selected the test succeeds otherwise it
> fails, but it is always there.

I followed the approach in lkdtm_DOUBLE_FAULT and others in
drivers/misc/lkdtm/bugs.c. I suppose it doesn't hurt to always build the
test irrespective of CONFIG_STRICT_KERNEL_RWX.

>
> Christophe