| Message ID | 20210713053113.4632-5-cmr@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use per-CPU temporary mappings for patching on Radix MMU | expand |
| Related | show |
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 ? 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. Christophe
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
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))
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(-)