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
Signed-off-by: Sven Schnelle <[email protected]>
---
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
Marco Elver <[email protected]> writes:
> On Fri, 14 May 2021 at 11:22, Sven Schnelle <[email protected]> wrote:
>>
>> Signed-off-by: Sven Schnelle <[email protected]>
>> ---
>> 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
On Fri, 14 May 2021 at 12:55, Sven Schnelle <[email protected]> wrote:
>
> Marco Elver <[email protected]> writes:
>
> > On Fri, 14 May 2021 at 11:22, Sven Schnelle <[email protected]> wrote:
> >>
> >> Signed-off-by: Sven Schnelle <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
---
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
On Fri, 14 May 2021 at 11:21, Sven Schnelle <[email protected]> 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
>
>
On Fri, 14 May 2021 at 11:22, Sven Schnelle <[email protected]> wrote:
>
> Signed-off-by: Sven Schnelle <[email protected]>
> ---
> 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