RE: [PATCH 3/3] mmc: block: ioctl: Add error aggregation flag

[Christian Loehle <CLoehle@hyperstone.com>]

-----Original Message-----
From: Ulf Hansson <ulf.hansson@linaro.org> 
Sent: Monday, May 15, 2023 3:56 PM
To: Christian Loehle <CLoehle@hyperstone.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Avri Altman <avri.altman@wdc.com>
Subject: Re: [PATCH 3/3] mmc: block: ioctl: Add error aggregation flag

>>
>> 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@hyperstone.com>
>> ---
>>  drivers/mmc/core/block.c       | 34 ++++++++++++++++++++++++++++++++--
>>  include/uapi/linux/mmc/ioctl.h |  2 ++
>>  2 files changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 
>> 35ff7101cbb1..386a508bd720 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -457,7 +457,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
>>                          sizeof(ic->response)))
>>                 return -EFAULT;
>>
>> -       if (!idata->ic.write_flag) {
>> +       if (!idata->ic.write_flag && idata->buf) {
> 
> If needed, this looks like it should be a separate change.

I'll retest, it was mostly about the new bit with e.g. a switch command to be more
explicit, it doesn't actually break anything to enter, but to emphasize
that write_flag != 0 does not imply idata->buf, which is counter-intuitive.
Will rework in v2

>
>>                 if (copy_to_user((void __user *)(unsigned long)ic->data_ptr,
>>                                  idata->buf, idata->buf_bytes))
>>                         return -EFAULT; @@ -610,13 +610,43 @@ static 
>> int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>>                 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)
>> +       if ((card->host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp &&
>> +                       !(idata->ic.write_flag & 
>> + MMC_AGGREGATE_PROG_ERRORS))
> 
> Do we really need a new flag for this? Can't we just collect the error code always for writes? Or collect the errors based upon a selection of commands?
> 
Strictly speaking no, i chose it for three reasons:
a) it's more flexible, don't have to hardcode some operations
b) it doesn't change current userspace interface, acts like a versioning of the ioctl interface.
c) it's easy to check if userspace makes use of it, one could even think of adding a WARN_ON if e.g.
sanitize is called without MMC_AGGREGATE_PROG_ERRORS as it might create a false sense of security -> is insecure.


>>                 return 0;
>>
>>         if (mmc_host_is_spi(card->host)) {
>>                 if (idata->ic.write_flag)
>>                         err = mmc_spi_err_check(card);
>>         }
>> +       /*
>> +        * We want to receive a meaningful R1 response for errors of detection
>> +        * type X, which are only set after the card has executed the command.
>> +        * In that case poll CMD13 until PROG is left and return the
>> +        * accumulated error bits.
>> +        * If we're lucky host controller has busy detection for R1B and
>> +        * this is just a single CMD13.
>> +        * We can abuse resp[1] as the post-PROG R1 here, as all commands
>> +        * that go through PRG have an R1 response and therefore only
>> +        * use resp[0].
>
> Hmm, for these cases, is the resp[0] containing any interesting information? Probably not, right?
> 
> In that case, why not override the resp[0], this should make the behaviour more consistent from user space point of view.
It doesn't affect mmc-utils and if we consider it the only client that's fair, but with thoughts about removing the flag
it felt a bit off tbh. The status and ready for data field will of course be non-sense. (I think I will overwrite the status fields
with the last polled status and only aggregate the error flags, to at least accomodate for that.)

>> +        */
>> +       else if (idata->ic.write_flag & MMC_AGGREGATE_PROG_ERRORS) {
>> +               unsigned long timeout = jiffies +
>> +                       msecs_to_jiffies(busy_timeout_ms);
>> +               bool done = false;
>> +               unsigned long delay_ms = 1;
>> +               u32 status;
>> +
>> +               do {
>> +                       done = time_after(jiffies, timeout);
>> +                       msleep(delay_ms++);
>> +                       err = __mmc_send_status(card, &status, 1);
>> +                       if (err)
>> +                               return err;
>> +                       idata->ic.response[1] |= status;
>> +               } while (R1_CURRENT_STATE(status) != R1_STATE_TRAN && !done);
>> +               if (done)
>> +                       return -ETIMEDOUT;
>> +       }
> 
> We have moved away from open-coding polling loops. Let's not introduce a new one. In fact, it looks a bit like we could re-use the
> mmc_blk_busy_cb() for this, as it seems to be collecting the error codes too.
> 
> In any case, let's at least make use of __mmc_poll_for_busy() to avoid the open-coding.
Makes sense.

>>         /* Ensure RPMB/R1B command has completed by polling with CMD13. */
>>         else if (idata->rpmb || r1b_resp)
>>                 err = mmc_poll_for_busy(card, busy_timeout_ms, false, 
>> diff --git a/include/uapi/linux/mmc/ioctl.h 
>> b/include/uapi/linux/mmc/ioctl.h index b2ff7f5be87b..a9d84ae8d57d 
>> 100644
>> --- a/include/uapi/linux/mmc/ioctl.h
>> +++ b/include/uapi/linux/mmc/ioctl.h
>> @@ -8,9 +8,11 @@
>>  struct mmc_ioc_cmd {
>>         /*
>>          * Direction of data: nonzero = write, zero = read.
>> +        * Bit 30 selects error aggregation post-PROG state.
>>          * Bit 31 selects 'Reliable Write' for RPMB.
>>          */
>>         int write_flag;
>> +#define MMC_AGGREGATE_PROG_ERRORS (1 << 30)
>>  #define MMC_RPMB_RELIABLE_WRITE (1 << 31)
>>
>>         /* Application-specific command.  true = precede with CMD55 */

So summarizing I will change:
- use __mmc_poll_for_busy with custom mmc_blk_busy_cb to aggregate error flags
- set non-error flags to last polled CMD13.
- put everything in resp[0]
- enable for all with write_flag != 0 or R1b response
- (use early return for spi write case)

Thanks to the both of you for taking a look at it.

Regards,
Christian

Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782