RE: [PATCHv3 1/1] mmc: block: ioctl: Add PROG-error aggregation

[Christian Loehle <CLoehle@hyperstone.com>]

[-- Attachment #1: Type: text/plain, Size: 5523 bytes --]



> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Donnerstag, 22. Juni 2023 11:46
> To: Christian Loehle <CLoehle@hyperstone.com>
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Adrian
> Hunter <adrian.hunter@intel.com>; Avri Altman <avri.altman@wdc.com>
> Subject: Re: [PATCHv3 1/1] mmc: block: ioctl: Add PROG-error aggregation
> 
> CAUTION: this mail comes from external!/ACHTUNG: Diese Mail kommt von
> extern!
> 
> On Tue, 20 Jun 2023 at 14:44, Christian Loehle <CLoehle@hyperstone.com>
> 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@hyperstone.com>
> > ---
> >  drivers/mmc/core/block.c   | 26 +++++++++++++++-----------
> >  drivers/mmc/core/mmc_ops.c | 14 +++++++-------
> > drivers/mmc/core/mmc_ops.h |  9 +++++++++
> >  3 files changed, 31 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> > e46330815484..c7e2b8ae58a9 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,24 @@ 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 RPMB/write/R1B execution errors */
> 
> Except for the other comments that I had on v2 (which isn't addressed in v3),
> I would like this comment to be extended a bit.
Sorry, could you elaborate on the comments I haven't addressed?
What I sent as v3 was what I understood from your comments.

> 
> More precisely, we somehow need to state that even if the host supports
> HW busy signaling (MMC_CAP_WAIT_WHILE_BUSY) we need to send a
> CMD13 to get the internal error status of the card.
Will do

> 
> > +       if (idata->rpmb || idata->ic.write_flag || r1b_resp) {
> > +               struct mmc_busy_data cb_data;
> > +
> > +               cb_data.card = card;
> > +               cb_data.retry_crc_err = false;
> > +               cb_data.aggregate_err_flags = true;
> > +               cb_data.busy_cmd = MMC_BUSY_IO;
> > +               cb_data.status = &idata->ic.response[0];
> > +               err = __mmc_poll_for_busy(card->host, 0, busy_timeout_ms,
> > +                               &mmc_busy_cb, &cb_data);
> > +
> > +       }
> >
> >         return err;
> >  }
> 
> [...]
> 
> Kind regards
> Uffe


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 10302 bytes --]