Re: [PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test

[Ganesh <ganeshgr@linux.ibm.com>]

On 9/26/20 1:27 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 04:01:22PM +0530, Ganesh Goudar wrote:
>> Add support to inject slb multihit errors, to test machine
>> check handling.
> Thank you for more tests in here!
>
>> Based on work by Mahesh Salgaonkar and Michal Suchánek.
>>
>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Cc: Michal Suchánek <msuchanek@suse.de>
> Should these be Co-developed-by: with S-o-b?
Sure, ill add.
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>>   drivers/misc/lkdtm/Makefile  |   4 ++
>>   drivers/misc/lkdtm/core.c    |   3 +
>>   drivers/misc/lkdtm/lkdtm.h   |   3 +
>>   drivers/misc/lkdtm/powerpc.c | 132 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 142 insertions(+)
>>   create mode 100644 drivers/misc/lkdtm/powerpc.c
>>
>> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
>> index c70b3822013f..6a82f407fbcd 100644
>> --- a/drivers/misc/lkdtm/Makefile
>> +++ b/drivers/misc/lkdtm/Makefile
>> @@ -11,6 +11,10 @@ lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
>>   lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
>>   lkdtm-$(CONFIG_LKDTM)		+= cfi.o
>>   
>> +ifeq ($(CONFIG_PPC64),y)
>> +lkdtm-$(CONFIG_LKDTM)		+= powerpc.o
>> +endif
> This can just be:
>
> lkdtm-$(CONFIG_PPC64)		+= powerpc.o
ok
>> +
>>   KASAN_SANITIZE_stackleak.o	:= n
>>   KCOV_INSTRUMENT_rodata.o	:= n
>>   
>> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
>> index a5e344df9166..8d5db42baa90 100644
>> --- a/drivers/misc/lkdtm/core.c
>> +++ b/drivers/misc/lkdtm/core.c
>> @@ -178,6 +178,9 @@ static const struct crashtype crashtypes[] = {
>>   #ifdef CONFIG_X86_32
>>   	CRASHTYPE(DOUBLE_FAULT),
>>   #endif
>> +#ifdef CONFIG_PPC64
>> +	CRASHTYPE(PPC_SLB_MULTIHIT),
>> +#endif
>>   };
>>   
>>   
>> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
>> index 8878538b2c13..b305bd511ee5 100644
>> --- a/drivers/misc/lkdtm/lkdtm.h
>> +++ b/drivers/misc/lkdtm/lkdtm.h
>> @@ -104,4 +104,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>>   /* cfi.c */
>>   void lkdtm_CFI_FORWARD_PROTO(void);
>>   
>> +/* powerpc.c */
>> +void lkdtm_PPC_SLB_MULTIHIT(void);
>> +
>>   #endif
>> diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
>> new file mode 100644
>> index 000000000000..d6db18444757
>> --- /dev/null
>> +++ b/drivers/misc/lkdtm/powerpc.c
>> @@ -0,0 +1,132 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
> Please #include "lkdtm.h" here to get the correct pr_fmt heading (and
> any future header adjustments).
Sure
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +
>> +static inline unsigned long get_slb_index(void)
>> +{
>> +	unsigned long index;
>> +
>> +	index = get_paca()->stab_rr;
>> +
>> +	/*
>> +	 * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
>> +	 */
>> +	if (index < (mmu_slb_size - 1))
>> +		index++;
>> +	else
>> +		index = SLB_NUM_BOLTED;
>> +	get_paca()->stab_rr = index;
>> +	return index;
>> +}
>> +
>> +#define slb_esid_mask(ssize)	\
>> +	(((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
>> +
>> +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
>> +					 unsigned long slot)
>> +{
>> +	return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
>> +}
>> +
>> +#define slb_vsid_shift(ssize)	\
>> +	((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
>> +
>> +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
>> +					 unsigned long flags)
>> +{
>> +	return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
>> +		((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
>> +}
>> +
>> +static void insert_slb_entry(char *p, int ssize)
>> +{
>> +	unsigned long flags, entry;
>> +
>> +	flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
>> +	preempt_disable();
>> +
>> +	entry = get_slb_index();
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
>> +			: "memory");
>> +
>> +	entry = get_slb_index();
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
>> +			: "memory");
>> +	preempt_enable();
>> +	p[0] = '!';
>> +}
> Can you add some comments to these helpers? It'll help people (me) with
> understanding what is actually being done here (and more importantly,
> what is _expected_ to happen).
Sure, ill add comments.
>> +
>> +static void inject_vmalloc_slb_multihit(void)
>> +{
>> +	char *p;
>> +
>> +	p = vmalloc(2048);
>> +	if (!p)
>> +		return;
>> +
>> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
>> +	vfree(p);
>> +}
>> +
>> +static void inject_kmalloc_slb_multihit(void)
>> +{
>> +	char *p;
>> +
>> +	p = kmalloc(2048, GFP_KERNEL);
>> +	if (!p)
>> +		return;
>> +
>> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
>> +	kfree(p);
>> +}
> It looks like the expected failure injection is actually the "p[0] = '!'" line in the
> "insert" helper? I would expect pr_info/pr_err wrappers, etc, as in
> other lkdtm tests.
Yes "p[0] = '!'" is the final step in this error injection, ill add 
comments.
> If this is the negative test, what does the positive test look like?
> e.g. in other lkdtm tests, first a positive test is done, then a
> negative: first run a legit function, then run a function from a bad
> location.

Yes, this is negative test, As SLB search is very fundamental thing in 
address translation and

used all the time, no positive test required here.

>> +
>> +static void insert_dup_slb_entry_0(void)
>> +{
>> +	unsigned long test_address = 0xC000000000000000;
>> +	volatile unsigned long *test_ptr;
>> +	unsigned long entry, i = 0;
>> +	unsigned long esid, vsid;
>> +
>> +	test_ptr = (unsigned long *)test_address;
>> +	preempt_disable();
>> +
>> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
>> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
>> +	entry = get_slb_index();
>> +
>> +	/* for i !=0 we would need to mask out the old entry number */
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (vsid),
>> +			  "r" (esid | entry)
>> +			: "memory");
>> +
>> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
>> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
>> +	entry = get_slb_index();
>> +
>> +	/* for i !=0 we would need to mask out the old entry number */
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (vsid),
>> +			  "r" (esid | entry)
>> +			: "memory");
>> +
>> +	pr_info("lkdtm: %s accessing test address 0x%lx: 0x%lx\n",
>> +		__func__, test_address, *test_ptr);
>> +
>> +	preempt_enable();
>> +}
> What does this do?

It tries to inject error in address range where most important kernel 
data structure may fall.

Mahesh and Michal Suchánek correct me if I am wrong.

>> +
>> +void lkdtm_PPC_SLB_MULTIHIT(void)
>> +{
>> +	if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
>> +		inject_vmalloc_slb_multihit();
>> +		inject_kmalloc_slb_multihit();
>> +		insert_dup_slb_entry_0();
>> +	}
>> +	pr_info("Recovered from SLB multihit. (Ignore this message on non HPTE machines)");
> Is this bad? If so, I'd expect pr_err("FAIL: ...") Can HPTE machines be
> detected so that an XFAIL can be emitted instead?

Sure, ill add XFAIL print in non HPTE case. And in case of failure we 
will not return

here, so if we hit this print line we are good.

> Since there are three (two?) distinct regions being tested, should these
> be separate tests? Right now there is no way to separate a vmalloc
> failure from a kmalloc failure, and no way to fail the first without
> hiding the result from the latter (or maybe the machine cannot survive
> this test? ... which should also be a comment.)
Sure, ill comment, And yes machine cannot survive these tests in case of 
failure to handle.
> And finally, assuming a successful test (or testing from a separate
> thread later), so there any state that needs to be restored (or cleaned
> up before doing the "insert" calls)?
No, there is nothing to be restored here, Thanks!
> Thanks!
>