RE: [PATCH 2/2] arm64: print alloc free paths for address in registers

[Maninder Singh]

Hi,

> 
>This path is used for a number of failures that might have nothing to do
>with a use-after-free, and from the trimmed example below it looks like
>this could significantly bloat the panic and potentially cause important
>information to be lost from the log, especially given the large number
>of GPRs arm64 has.
> 
>Given that, I suspect this is not something we want enabled by default.
>

Yes it will add a lot of logs in case of normal die also.
But it can suggest free and alloc paths which can help in some debugging.

>When is this logging enabled? I assume the kernel doesn't always record
>the alloc/free paths. Is there a boot-time option to control this?
> 

if SLUB_DEBUG_ON is enabled at build time then it is enabled from boot,
otherwise it can be enabled by kernel command line parameter of "slub_debug=u"
if SLUB_DEBUG is enabled.


>How many lines does this produce on average?
> 

16 traces at max for alloc and 16 for free path.
so in total for slab object 34 lines will be printed,
otherwise one line for each object info of vmalloc.

>>  
>> +void __show_regs_alloc_free(struct pt_regs *regs)
>> +{
>> +        int i;
>> +
>> +        /* check for x0 - x29 only */
> 
>Why x29? The AAPCS says that's the frame pointer, so much like the SP it
>shouldn't point to a heap object.

yes x29 can be ignored.

> 
>> +        for (i = 0; i <= 29; i++) {
>> +                pr_alert("Register x%d information:", i);
>> +                mem_dump_obj((void *)regs->regs[i]);
>> +        }
>> +}
> 
>The pr_alert() is unconditional -- can mem_dumpo_obj() never be
>disabled?
>

mem_dump_obj is dependent on CONFIG_PRINTK

>What loglevel does mem_dump_obj() use? Generally we try to keep that
>matched, so I'm surprised it isn't taken as a parameter.
> 

loglevel of mem_dump_obj is pr_cont, so it will be with the caller's loglevel

/**
 * mem_dump_obj - Print available provenance information
 * @object: object for which to find provenance information.
 *
 * This function uses pr_cont(), so that the caller is expected to have
 * printed out whatever preamble is appropriate.  
 
 
But loglvel will be changed to pr_info when it goes for printing of traces.

void kmem_dump_obj(void *object)
{
....
                pr_cont("\n");
        for (i = 0; i < ARRAY_SIZE(kp.kp_stack); i++) {
                if (!kp.kp_stack[i])
                        break;
                pr_info("    %pS\n", kp.kp_stack[i]);
        }

...

>> +
>>  void show_regs(struct pt_regs *regs)
>>  {
>>          __show_regs(regs);
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index a05d34f0e82a..cb4858c6e57b 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -104,6 +104,7 @@ static int __die(const char *str, int err, struct pt_regs *regs)
>>  
>>          print_modules();
>>          show_regs(regs);
>> +        __show_regs_alloc_free(regs);
> 
>As above, I'm not sure this is the right place to put this. We can get
>here for reasons other than UAF, and I'm sure we can trigger panics via
>UAF without going via this.
> 

Adding call here, because we though in case of use after free __die will be called.
due to unhandled page fault of 0x6b6b6 MAGIC value. thats why picked this place.

Thanks,
Maninder Singh