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

[Christian Loehle]

> -----Original Message-----
> From: Avri Altman <Avri.Altman@xxxxxxx>
> Sent: Wednesday, June 21, 2023 9:31 AM
> To: Christian Loehle <CLoehle@xxxxxxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Ulf Hansson <ulf.hansson@xxxxxxxxxx>; Adrian
> Hunter <adrian.hunter@xxxxxxxxx>
> Subject: RE: [PATCHv3 1/1] mmc: block: ioctl: Add PROG-error aggregation
>
> CAUTION: this mail comes from external!/ACHTUNG: Diese Mail kommt von
> extern!
>
> > 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>
> Acked-by: Avri Altman <avri.altman@xxxxxxx>
>
> > ---
> >  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 */
> > +     if (idata->rpmb || idata->ic.write_flag || r1b_resp) {
> AFAIK write_flag  and r1b_resp are set together (in mmc-utils that is)?
Not sure what you're trying to say, that write_flag -> r1b_resp? If so that is not true for all cases, e.g. CMD56
> As for rpmb read operations you were pondering about - the rpmb read-
> counter is one example, because you use it to sign write operations.
But the read counter (the actual read direction part), doesn't really need post transfer polling IMO.
If the card encounters a problem during that operation, it hopefully doesn't send valid counter read packet response.
The read counter doesn't change any state inside the card, and if it does for some vendor-specific reason, I would hope it does this before the read data is executed.
So I would say we're in the clear without polling.
Checking the validity of the received read counter packet data is responsibility of the ioctl-client, and I would say they already have everything they need.

> So restoring all rpmb operations is in place - all should be monitored.
>
> > +             struct mmc_busy_data cb_data;
> Maybe use designated initializing?

Can do that, thanks.
I will wait for Uffe's comment about the style before resubmitting.

Regards,
Christian

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