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

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(-)

--
2.37.3

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

On Wed, 28 Jun 2023 at 08:47, Christian Loehle <CLoehle@hyperstone.com> wrote:
>
>
>
> > -----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.

No problem, it's probably me that was not clear enough.

Anyway, to help mode this forward, let me amend the patch and submit a
new version of it. Then you can have a look and confirm that it looks
good to you.

[...]

Kind regards
Uffe