Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Mime-Version: 1.0
Subject: RE: [PATCH 2/2] arm64: print alloc free paths for address in
 registers
Reply-To: maninder1.s@samsung.com
Sender: Maninder Singh <maninder1.s@samsung.com>
From:   Maninder Singh <maninder1.s@samsung.com>
To:     Mark Rutland <mark.rutland@arm.com>
CC:     "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will@kernel.org" <will@kernel.org>,
        "pcc@google.com" <pcc@google.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "0x7f454c46@gmail.com" <0x7f454c46@gmail.com>,
        "amit.kachhap@arm.com" <amit.kachhap@arm.com>,
        "james.morse@arm.com" <james.morse@arm.com>,
        "vincenzo.frascino@arm.com" <vincenzo.frascino@arm.com>,
        "nsaenzjulienne@suse.de" <nsaenzjulienne@suse.de>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "samitolvanen@google.com" <samitolvanen@google.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        AMIT SAHRAWAT <a.sahrawat@samsung.com>,
        Vaneet Narang <v.narang@samsung.com>
In-Reply-To: <20210324115113.GA19135@C02TD0UTHF1T.local>
Message-ID: <20210324144031epcms5p30f848873ba3a4e20f4eb4e5d13d381f7@epcms5p3>
Date:   Wed, 24 Mar 2021 20:10:31 +0530
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
References: <20210324115113.GA19135@C02TD0UTHF1T.local>
        <1616568899-986-1-git-send-email-maninder1.s@samsung.com>
        <1616568899-986-2-git-send-email-maninder1.s@samsung.com>
        <CGME20210324065516epcas5p450958011b69f5941e8f2bc993b82b904@epcms5p3>
Precedence: bulk

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