[RFC] minor kfence patches

[RFC] minor kfence patches

[Sven Schnelle]

i'm currently looking into adding support for KFENCE to the s390
architecture. So far everything is straightforward, and i get the
kfence testsuite to pass, which is good! :)

One minor thing i encountered is that for a translation exception,
s390 only reports the page address, but not the complete address. I
worked around that by adding a function to kfence which allows to mask
out certain bits during unit testing. I wonder whether that should be a
weak function that can be implemented by architectures if required, some
kconfig option, or some other way?

The other thing is that s390 (and some other architectures) has different
address spaces for kernel and user space, so the decision whether an
address belongs to user or kernel space cannot be made by just looking
at the address. I added a small if (user_mode(regs)) check to
kfence_handle_page_fault(). But this could of also be done in the
architecture specific code.

What do you think?

Thanks,
Sven

[PATCH 1/2] kfence: add function to mask address bits

[Sven Schnelle]

s390 only reports the page address during a translation fault.
To make the kfence unit tests pass, add a function that might
be implemented by architectures to mask out address bits.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 include/linux/kfence.h  | 1 +
 mm/kfence/core.c        | 5 +++++
 mm/kfence/kfence_test.c | 6 +++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/kfence.h b/include/linux/kfence.h
index a70d1ea03532..2e15f4c4ee95 100644
--- a/include/linux/kfence.h
+++ b/include/linux/kfence.h
@@ -199,6 +199,7 @@ static __always_inline __must_check bool kfence_free(void *addr)
  * present, so that the kernel can proceed.
  */
 bool __must_check kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs);
+unsigned long kfence_arch_mask_addr(unsigned long addr);
 
 #else /* CONFIG_KFENCE */
 
diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index e18fbbd5d9b4..bc15e3cb71d5 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE
 #endif
 #define MODULE_PARAM_PREFIX "kfence."
 
+unsigned long __weak kfence_arch_mask_addr(unsigned long addr)
+{
+	return addr;
+}
+
 static int param_set_sample_interval(const char *val, const struct kernel_param *kp)
 {
 	unsigned long num;
diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
index 4acf4251ee04..9ec572991014 100644
--- a/mm/kfence/kfence_test.c
+++ b/mm/kfence/kfence_test.c
@@ -82,6 +82,7 @@ static const char *get_access_type(const struct expect_report *r)
 /* Check observed report matches information in @r. */
 static bool report_matches(const struct expect_report *r)
 {
+	unsigned long addr = (unsigned long)r->addr;
 	bool ret = false;
 	unsigned long flags;
 	typeof(observed.lines) expect;
@@ -131,22 +132,25 @@ static bool report_matches(const struct expect_report *r)
 	switch (r->type) {
 	case KFENCE_ERROR_OOB:
 		cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r));
+		addr = kfence_arch_mask_addr(addr);
 		break;
 	case KFENCE_ERROR_UAF:
 		cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r));
+		addr = kfence_arch_mask_addr(addr);
 		break;
 	case KFENCE_ERROR_CORRUPTION:
 		cur += scnprintf(cur, end - cur, "Corrupted memory at");
 		break;
 	case KFENCE_ERROR_INVALID:
 		cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r));
+		addr = kfence_arch_mask_addr(addr);
 		break;
 	case KFENCE_ERROR_INVALID_FREE:
 		cur += scnprintf(cur, end - cur, "Invalid free of");
 		break;
 	}
 
-	cur += scnprintf(cur, end - cur, " 0x%p", (void *)r->addr);
+	cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr);
 
 	spin_lock_irqsave(&observed.lock, flags);
 	if (!report_available())
-- 
2.25.1

[PATCH 2/2] kfence: only handle kernel mode faults

[Sven Schnelle]

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 mm/kfence/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index bc15e3cb71d5..161df492750c 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
 	enum kfence_error_type error_type;
 	unsigned long flags;
 
+	if (user_mode(regs))
+		return false;
+
 	if (!is_kfence_address((void *)addr))
 		return false;
 
-- 
2.25.1

Re: [PATCH 2/2] kfence: only handle kernel mode faults

[Marco Elver]

On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote:
>
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  mm/kfence/core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index bc15e3cb71d5..161df492750c 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
>         enum kfence_error_type error_type;
>         unsigned long flags;
>
> +       if (user_mode(regs))
> +               return false;
> +

I don't think it's required on all architectures, correct? If so, I
think this should be part of the arch-specific code, i.e. just do "if
(user_mode(regs) && kfence_handle_page_fault(...))" or similar.
Because otherwise we'll wonder in future why we ever needed this, and
e.g. determine it's useless and remove it again. ;-) Either that, or a
comment. But I'd prefer to just keep it in the arch-specific code if
required, because it seems to be the exception rather than the norm.

Thanks,
-- Marco

Re: [PATCH 1/2] kfence: add function to mask address bits

[Marco Elver]

Thanks for trying to get KFENCE on s390.

On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote:
>
> s390 only reports the page address during a translation fault.
> To make the kfence unit tests pass, add a function that might
> be implemented by architectures to mask out address bits.

The point of the test is to test the expected behaviour. And s390
certainly isn't behaving as we'd expect, because we really ought to
see the precise address to facilitate debugging. Granted, by default
KFENCE prints hashed pointers, but with no_hash_pointers we still want
to see the precise address.

Is there any way to make s390 give us precise addresses?

Of course if you say this deviation is reasonable, see my suggestions below.

> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  include/linux/kfence.h  | 1 +
>  mm/kfence/core.c        | 5 +++++
>  mm/kfence/kfence_test.c | 6 +++++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> index a70d1ea03532..2e15f4c4ee95 100644
> --- a/include/linux/kfence.h
> +++ b/include/linux/kfence.h
> @@ -199,6 +199,7 @@ static __always_inline __must_check bool kfence_free(void *addr)
>   * present, so that the kernel can proceed.
>   */
>  bool __must_check kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs);
> +unsigned long kfence_arch_mask_addr(unsigned long addr);

I think this should not be part of the public interface, as commented below.

>  #else /* CONFIG_KFENCE */
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index e18fbbd5d9b4..bc15e3cb71d5 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE
>  #endif
>  #define MODULE_PARAM_PREFIX "kfence."
>
> +unsigned long __weak kfence_arch_mask_addr(unsigned long addr)
> +{
> +       return addr;
> +}

I don't think this belongs here, because it's test-specific,
furthermore if possible we'd like to put all arch-specific code into
<asm/kfence.h> (whether or not your arch will have 'static inline'
functions only, like x86 and arm64, or not is up to you).

Because I don't see this function being terribly complex, also let's
just make it a macro.

Then in kfence_test.c, we can have:

#ifndef kfence_test_mask_address
#define kfence_test_mask_address(addr) (addr)
#endif

and then have it include <asm/kfence.h>. And in your <asm/kfence.h>
you can simply say:

#define kfence_test_mask_address(addr) (.........)

It also avoids having to export kfence_test_mask_address, because
kfence_test can be built as a module.

>  static int param_set_sample_interval(const char *val, const struct kernel_param *kp)
>  {
>         unsigned long num;
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index 4acf4251ee04..9ec572991014 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -82,6 +82,7 @@ static const char *get_access_type(const struct expect_report *r)
>  /* Check observed report matches information in @r. */
>  static bool report_matches(const struct expect_report *r)
>  {
> +       unsigned long addr = (unsigned long)r->addr;
>         bool ret = false;
>         unsigned long flags;
>         typeof(observed.lines) expect;
> @@ -131,22 +132,25 @@ static bool report_matches(const struct expect_report *r)
>         switch (r->type) {
>         case KFENCE_ERROR_OOB:
>                 cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r));
> +               addr = kfence_arch_mask_addr(addr);
>                 break;
>         case KFENCE_ERROR_UAF:
>                 cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r));
> +               addr = kfence_arch_mask_addr(addr);
>                 break;
>         case KFENCE_ERROR_CORRUPTION:
>                 cur += scnprintf(cur, end - cur, "Corrupted memory at");
>                 break;
>         case KFENCE_ERROR_INVALID:
>                 cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r));
> +               addr = kfence_arch_mask_addr(addr);
>                 break;
>         case KFENCE_ERROR_INVALID_FREE:
>                 cur += scnprintf(cur, end - cur, "Invalid free of");
>                 break;
>         }
>
> -       cur += scnprintf(cur, end - cur, " 0x%p", (void *)r->addr);
> +       cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr);

The rest here looks reasonable if you think there's no way to get s390
to give us precise addresses.

Thanks,
-- Marco

Re: [PATCH 2/2] kfence: only handle kernel mode faults

[Sven Schnelle]

Marco Elver <elver@google.com> writes:

> On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote:
>>
>> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
>> ---
>>  mm/kfence/core.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
>> index bc15e3cb71d5..161df492750c 100644
>> --- a/mm/kfence/core.c
>> +++ b/mm/kfence/core.c
>> @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
>>         enum kfence_error_type error_type;
>>         unsigned long flags;
>>
>> +       if (user_mode(regs))
>> +               return false;
>> +
>
> I don't think it's required on all architectures, correct? If so, I
> think this should be part of the arch-specific code, i.e. just do "if
> (user_mode(regs) && kfence_handle_page_fault(...))" or similar.
> Because otherwise we'll wonder in future why we ever needed this, and
> e.g. determine it's useless and remove it again. ;-) Either that, or a
> comment. But I'd prefer to just keep it in the arch-specific code if
> required, because it seems to be the exception rather than the norm.

Ok, that's fine, i add it to our code then.

Thanks
Sven

Re: [RFC] minor kfence patches

[Marco Elver]

On Fri, 14 May 2021 at 11:21, Sven Schnelle <svens@linux.ibm.com> wrote:
>
> i'm currently looking into adding support for KFENCE to the s390
> architecture. So far everything is straightforward, and i get the
> kfence testsuite to pass, which is good! :)

Nice to see KFENCE being added to more architectures.

> One minor thing i encountered is that for a translation exception,
> s390 only reports the page address, but not the complete address. I
> worked around that by adding a function to kfence which allows to mask
> out certain bits during unit testing. I wonder whether that should be a
> weak function that can be implemented by architectures if required, some
> kconfig option, or some other way?

I've commented on the other patches.


Thanks,
-- Marco

> The other thing is that s390 (and some other architectures) has different
> address spaces for kernel and user space, so the decision whether an
> address belongs to user or kernel space cannot be made by just looking
> at the address. I added a small if (user_mode(regs)) check to
> kfence_handle_page_fault(). But this could of also be done in the
> architecture specific code.
>
> What do you think?
>
> Thanks,
> Sven
>
>

Re: [PATCH 2/2] kfence: only handle kernel mode faults

[Marco Elver]

On Fri, 14 May 2021 at 12:55, Sven Schnelle <svens@linux.ibm.com> wrote:
>
> Marco Elver <elver@google.com> writes:
>
> > On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote:
> >>
> >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> >> ---
> >>  mm/kfence/core.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> >> index bc15e3cb71d5..161df492750c 100644
> >> --- a/mm/kfence/core.c
> >> +++ b/mm/kfence/core.c
> >> @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
> >>         enum kfence_error_type error_type;
> >>         unsigned long flags;
> >>
> >> +       if (user_mode(regs))
> >> +               return false;
> >> +
> >
> > I don't think it's required on all architectures, correct? If so, I
> > think this should be part of the arch-specific code, i.e. just do "if
> > (user_mode(regs) && kfence_handle_page_fault(...))" or similar.

Ah, this should have obviously been "if (!user_mode(regs) &&
kfence_handle_page_fault(...))", but I think you would have caught
that anyway. ;-)

> > Because otherwise we'll wonder in future why we ever needed this, and
> > e.g. determine it's useless and remove it again. ;-) Either that, or a
> > comment. But I'd prefer to just keep it in the arch-specific code if
> > required, because it seems to be the exception rather than the norm.
>
> Ok, that's fine, i add it to our code then.

Sounds good.

Thanks,
-- Marco

> Thanks
> Sven

Re: [PATCH 1/2] kfence: add function to mask address bits

[Sven Schnelle]

Marco Elver <elver@google.com> writes:

>> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
>> index e18fbbd5d9b4..bc15e3cb71d5 100644
>> --- a/mm/kfence/core.c
>> +++ b/mm/kfence/core.c
>> @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE
>>  #endif
>>  #define MODULE_PARAM_PREFIX "kfence."
>>
>> +unsigned long __weak kfence_arch_mask_addr(unsigned long addr)
>> +{
>> +       return addr;
>> +}
>
> I don't think this belongs here, because it's test-specific,
> furthermore if possible we'd like to put all arch-specific code into
> <asm/kfence.h> (whether or not your arch will have 'static inline'
> functions only, like x86 and arm64, or not is up to you).
>
> Because I don't see this function being terribly complex, also let's
> just make it a macro.
>
> Then in kfence_test.c, we can have:
>
> #ifndef kfence_test_mask_address
> #define kfence_test_mask_address(addr) (addr)
> #endif
>
> and then have it include <asm/kfence.h>. And in your <asm/kfence.h>
> you can simply say:
>
> #define kfence_test_mask_address(addr) (.........)
>
> It also avoids having to export kfence_test_mask_address, because
> kfence_test can be built as a module.
 
Ok, i'll change my patch accordingly. Thanks!

Sven

Re: [PATCH 1/2] kfence: add function to mask address bits

[Marco Elver]

On Fri, 14 May 2021 at 13:03, Sven Schnelle <svens@linux.ibm.com> wrote:
>
> Marco Elver <elver@google.com> writes:
>
> >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> >> index e18fbbd5d9b4..bc15e3cb71d5 100644
> >> --- a/mm/kfence/core.c
> >> +++ b/mm/kfence/core.c
> >> @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE
> >>  #endif
> >>  #define MODULE_PARAM_PREFIX "kfence."
> >>
> >> +unsigned long __weak kfence_arch_mask_addr(unsigned long addr)
> >> +{
> >> +       return addr;
> >> +}
> >
> > I don't think this belongs here, because it's test-specific,
> > furthermore if possible we'd like to put all arch-specific code into
> > <asm/kfence.h> (whether or not your arch will have 'static inline'
> > functions only, like x86 and arm64, or not is up to you).
> >
> > Because I don't see this function being terribly complex, also let's
> > just make it a macro.
> >
> > Then in kfence_test.c, we can have:
> >
> > #ifndef kfence_test_mask_address
> > #define kfence_test_mask_address(addr) (addr)
> > #endif
> >
> > and then have it include <asm/kfence.h>. And in your <asm/kfence.h>
> > you can simply say:
> >
> > #define kfence_test_mask_address(addr) (.........)
> >
> > It also avoids having to export kfence_test_mask_address, because
> > kfence_test can be built as a module.
>
> Ok, i'll change my patch accordingly. Thanks!

Sounds good. Also please add a brief comment on top of the
"kfence_test_mask_address" part in kfence_test, like "/* May be
overridden by <asm/kfence.h>. */" -- we have something similar in
mm/kfence/report.c. Also, I think we want to call the macro
"arch_kfence_test_address" -- the "mask" part is very much
arch-dependent, and might not even be a mask on some other weird
hypothetical architecture.

Thanks,
-- Marco