RE: [PATCHv2 2/2] mmc: block: ioctl: Add PROG-error aggregation

[Christian Loehle]

>>
>> Userspace currently has no way of checking for error bits of detection 
>> mode X. These are error bits that are only detected by the card when 
>> executing the command. For e.g. a sanitize operation this may be 
>> minutes after the RSP was seen by the host.
>>
>> Currently userspace programs cannot see these error bits reliably.
>> They could issue a multi ioctl cmd with a CMD13 immediately following 
>> it, but since errors of detection mode X are automatically cleared 
>> (they are all clear condition B).
>> mmc_poll_for_busy of the first ioctl may have already hidden such an 
>> error flag.
>>
>> In case of the security operations: sanitize, secure erases and RPMB 
>> writes, this could lead to the operation not being performed 
>> successfully by the card with the user not knowing.
>> If the user trusts that this operation is completed (e.g. their data 
>> is sanitized), this could be a security issue.
>> An attacker could e.g. provoke a eMMC (VCC) flash fail, where a 
>> successful sanitize of a card is not possible. A card may move out of 
>> PROG state but issue a bit 19 R1 error.
>>
>> This patch therefore will also have the consequence of a mmc-utils 
>> patch, which enables the bit for the security-sensitive operations.
>>
>> Signed-off-by: Christian Loehle <cloehle@xxxxxxxxxxxxxx>
>> ---
>>  drivers/mmc/core/block.c   | 17 ++++++-----------
>>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++++++++++++++-  
>> drivers/mmc/core/mmc_ops.h |  3 +++
>>  3 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 
>> e46330815484..44c1b2825032 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -470,7 +470,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>>         struct mmc_data data = {};
>>         struct mmc_request mrq = {};
>>         struct scatterlist sg;
>> -       bool r1b_resp, use_r1b_resp = false;
>> +       bool r1b_resp;
>>         unsigned int busy_timeout_ms;
>>         int err;
>>         unsigned int target_part;
>> @@ -551,8 +551,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>>         busy_timeout_ms = idata->ic.cmd_timeout_ms ? : MMC_BLK_TIMEOUT_MS;
>>         r1b_resp = (cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B;
>>         if (r1b_resp)
>> -               use_r1b_resp = mmc_prepare_busy_cmd(card->host, &cmd,
>> -                                                   busy_timeout_ms);
>> +               mmc_prepare_busy_cmd(card->host, &cmd, 
>> + busy_timeout_ms);
>>
>>         mmc_wait_for_req(card->host, &mrq);
>>         memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); @@ 
>> -605,19 +604,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>>         if (idata->ic.postsleep_min_us)
>>                 usleep_range(idata->ic.postsleep_min_us, 
>> idata->ic.postsleep_max_us);
>>
>> -       /* No need to poll when using HW busy detection. */
>> -       if ((card->host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>> -               return 0;
>> -
>>         if (mmc_host_is_spi(card->host)) {
>>                 if (idata->ic.write_flag || r1b_resp || cmd.flags & MMC_RSP_SPI_BUSY)
>>                         return mmc_spi_err_check(card);
>>                 return err;
>>         }
>> -       /* Ensure RPMB/R1B command has completed by polling with CMD13. */
>> -       if (idata->rpmb || r1b_resp)
>> -               err = mmc_poll_for_busy(card, busy_timeout_ms, false,
>> -                                       MMC_BUSY_IO);
>> +       /* Poll for write/R1B execution errors */
>> +       if (idata->ic.write_flag || r1b_resp)
>
> Earlier we polled for requests that were targeted to rpmb, no matter if they were write or reads. Are you intentionally changing this? If so, can you explain why?
> 
Will re-introduce. I cant really think of a reason right now to do this after rpmb reads, but thats a different story.

>> +               err = mmc_poll_for_busy_err_flags(card, busy_timeout_ms, false,
>> +                                       MMC_BUSY_IO, 
>> + &idata->ic.response[0]);
>
> I think it's better to extend the mmc_blk_busy_cb, rather than introducing an entirely new polling function.
> 
> Then you can call __mmc_poll_for_busy() here instead.

Not sure if I understood you right, but I will send a new version with __mmc_poll_for_busy call directly.
It does feel a bit more awkward, at least to me, because both mmc_blk_busy_cb nor mmc_busy_data are currently only in mmc_ops.c

Anyway, both versions "extend the mmc_blk_busy_cb", so I'm not sure if I understood you correctly, we will see.
I may also just send both and you pick whichever you prefer.

Regards,
Christian

Attachment: smime.p7s
Description: S/MIME cryptographic signature