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 <c...@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
