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

[Ulf Hansson]

On Wed, 5 Apr 2023 at 13:57, Christian Löhle <CLoehle@xxxxxxxxxxxxxx> wrote:
>
> 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       | 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.

>                 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?

>                 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.

> +        */
> +       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.

>         /* 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 */

Kind regards
Uffe