linux-kernel - RE: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CY4PR1801MB20702A51343093FD43A53A50DEF20@CY4PR1801MB2070.namprd18.prod.outlook.com>
Date:   Thu, 3 Dec 2020 05:41:08 +0000
From:   Bhaskara Budiredla <bbudiredla@...vell.com>
To:     Kees Cook <keescook@...omium.org>
CC:     "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "ccross@...roid.com" <ccross@...roid.com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "Sunil Kovvuri Goutham" <sgoutham@...vell.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "outgoing2/0000-cover-letter.patch@...b-0016f401.pphosted.com" 
        <outgoing2/0000-cover-letter.patch@...b-0016f401.pphosted.com>
Subject: RE: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on
 pstore/blk



>-----Original Message-----
>From: Kees Cook <keescook@...omium.org>
>Sent: Thursday, December 3, 2020 1:02 AM
>To: Bhaskara Budiredla <bbudiredla@...vell.com>
>Cc: ulf.hansson@...aro.org; ccross@...roid.com; tony.luck@...el.com; Sunil
>Kovvuri Goutham <sgoutham@...vell.com>; linux-mmc@...r.kernel.org;
>linux-kernel@...r.kernel.org; outgoing2/0000-cover-letter.patch@...b-
>0016f401.pphosted.com
>Subject: Re: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on
>pstore/blk
>
>On Wed, Dec 02, 2020 at 06:36:21AM +0000, Bhaskara Budiredla wrote:
>> >From: Kees Cook <keescook@...omium.org> On Mon, Nov 23, 2020 at
>> >04:49:24PM +0530, Bhaskara Budiredla wrote:
>> >Why isn't this just written as:
>> >
>> >config MMC_PSTORE
>> >	bool "Log panic/oops to a MMC buffer"
>> >	depends on MMC_BLOCK
>> >	select PSTORE_BLK
>> >	help
>> >	  This option will let you create platform backend to store kmsg
>> >	  crash dumps to a user specified MMC device. This is primarily
>> >	  based on pstore/blk.
>> >
>>
>> The idea was to compile MMC_PSTORE as part of MMC_BLOCK driver,
>> provided it is optionally enabled.
>> The above arrangement compiles MMC_PSTORE as module: if
>> (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == m)
>> as static:     if (CONFIG_MMC_PSTORE_BACKEND == y &&
>CONFIG_MMC_BLOCK == y)
>
>Ah, okay. If it's a tri-state, wouldn't it track CONFIG_MMC_BLOCK's state? As
>in, does this work:
>

Yes, it's a tri-state but not compiled as a separate driver. 

>config MMC_PSTORE
>	tristate "Log panic/oops to a MMC buffer"
>	depends on MMC_BLOCK
>	select PSTORE_BLK
>	help
>	  This option will let you create platform backend to store kmsg
>	  crash dumps to a user specified MMC device. This is primarily
>	  based on pstore/blk.
>

No, this will cause problems for MMC_PSTORE=m and MMC_BLOCK=y
MMC_PSTORE automatically have to be selected as module or static
based on MMC_BLOCK selection. There were couple of function calls
from the code in MMC_BLOCK to MMC_PSTORE. Uffe prefers them
to be unconditional calls (as per discussion in v1).  

>> >> +	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
>> >> +		return;
>> >
>> >Why isn't this just strcmp()?
>>
>> The mmc disk name (disk_name) doesn't include the partition number.
>> strncmp is restricted to something like /dev/mmcblk0, it doesn't cover full
>/dev/mmcblk0pn.
>> The partition number check is carried out in the next statement.
>
>Okay, gotcha; thanks!
>
>> >> +	dev->flags = PSTORE_FLAGS_DMESG;
>> >
>> >Can't this support more than just DMESG? I don't see anything specific to
>that.
>> >This is using pstore/zone ultimately, which can support whatever
>> >frontends it needs to.
>>
>> Yes, as of now the support is only for DMESG. We will extend this to
>> other frontends on need basis.
>
>Okay -- I assume this has mostly to do with not having erasure (below).
>
>> >> +	dev->erase = NULL;
>> >
>> >No way to remove the records?
>>
>> Yes, at this time, no removal of records.
>
>Okay. (I think this might be worth mentioning in docs somewhere.)
>

Would it be sufficient to add corresponding notes to the commit message? 

>--
>Kees Cook