linux-kernel - Re: [PATCH] mmc: check EXT_CSD_PARTITION_SETTING

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Aug 2014 13:46:34 +0200
From:	Ulf Hansson <ulf.hansson@...aro.org>
To:	Grégory Soutadé <gsoutade@...tion.com>
Cc:	Chris Ball <chris@...ntf.net>,
	Seungwon Jeon <tgih.jun@...sung.com>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mmc: check EXT_CSD_PARTITION_SETTING_COMPLETED before
 creating partitions

On 13 August 2014 11:20, Grégory Soutadé <gsoutade@...tion.com> wrote:
> Le 13/08/2014 10:36, Ulf Hansson a écrit :
>> On 17 July 2014 16:57, Grégory Soutadé <gsoutade@...tion.com> wrote:
>>> Create MMC general purpose partitions only if
>>> EXT_CSD_PARTITION_SETTING_COMPLETED bit is set.
>>> Some tools may set general purpose partition size(s) but fail or stop
>>> without finalize.
>>> Another case is to set invalid partition size(s).
>>>
>>> Signed-off-by: Grégory Soutadé <gsoutade@...tion.com>
>>> ---
>>>  drivers/mmc/core/mmc.c  |   15 +++++++++++----
>>>  include/linux/mmc/mmc.h |    2 ++
>>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>>
>>> From commit b6603fe574af289dbe9eb9fb4c540bca04f5a053 in master linux tree.
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 793c6f7..b9fe211 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -471,10 +471,17 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>>                                 ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3];
>>>                                 part_size *= (size_t)(hc_erase_grp_sz *
>>>                                         hc_wp_grp_sz);
>>> -                               mmc_part_add(card, part_size << 19,
>>> -                                       EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
>>> -                                       "gp%d", idx, false,
>>> -                                       MMC_BLK_DATA_AREA_GP);
>>> +                               if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED] &
>>> +                               EXT_CSD_PART_SETTING_COMPLETED) {
>>
>> Some minor comments here.
>>
>> I think you could move this if statement up and into the previous "if"
>> where we check for "ext_csd[EXT_CSD_PARTITION_SUPPORT] &
>> EXT_CSD_PART_SUPPORT_PART_EN".
>>
>> Additionally, please run checkpatch.
>>
>> Kind regards
>> Uffe
>
> Hello,
>
> I didn't put the if statement above to warn user in case of bad partitioning.
> I don't want the kernel to silently ignore this error.

Fair enough.

Still I am wondering whether hc_erase_grp_sz, hc_wp_grp_sz and
enhanced_area_en should be updated if
EXT_CSD_PARTITION_SETTING_COMPLETED isn't set.  That's the case in
your patch.

>
> checkpatch has been run before sending this patch, I know there are two lines
> with two extra characters, but names used here are quite long. I hope it will
> not block upstream inclusion.

The mmc_read_ext_csd() is not one of the nicest piece of code, but for
sure we should not move on making it worse. If you need to move code
into separate function to prevent checkpatch warnings, please do so.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/