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

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

[Christian Loehle]

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

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 */
+	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;
 }
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 3b3adbddf664..15d8b806c670 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -54,11 +54,6 @@ static const u8 tuning_blk_pattern_8bit[] = {
 	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
 };

-struct mmc_busy_data {
-	struct mmc_card *card;
-	bool retry_crc_err;
-	enum mmc_busy_cmd busy_cmd;
-};

 struct mmc_op_cond_busy_data {
 	struct mmc_host *host;
@@ -457,14 +452,15 @@ int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
 	return mmc_switch_status_error(card->host, status);
 }

-static int mmc_busy_cb(void *cb_data, bool *busy)
+int mmc_busy_cb(void *cb_data, bool *busy)
 {
 	struct mmc_busy_data *data = cb_data;
 	struct mmc_host *host = data->card->host;
 	u32 status = 0;
 	int err;

-	if (data->busy_cmd != MMC_BUSY_IO && host->ops->card_busy) {
+	if (data->busy_cmd != MMC_BUSY_IO && host->ops->card_busy &&
+			!data->aggregate_err_flags) {
 		*busy = host->ops->card_busy(host);
 		return 0;
 	}
@@ -477,6 +473,9 @@ static int mmc_busy_cb(void *cb_data, bool *busy)
 	if (err)
 		return err;

+	if (data->aggregate_err_flags)
+		*data->status = R1_STATUS(*data->status) | status;
+
 	switch (data->busy_cmd) {
 	case MMC_BUSY_CMD6:
 		err = mmc_switch_status_error(host, status);
@@ -549,6 +548,7 @@ int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,

 	cb_data.card = card;
 	cb_data.retry_crc_err = retry_crc_err;
+	cb_data.aggregate_err_flags = false;
 	cb_data.busy_cmd = busy_cmd;

 	return __mmc_poll_for_busy(host, 0, timeout_ms, &mmc_busy_cb, &cb_data);
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 09ffbc00908b..a57751b83f19 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -18,6 +18,14 @@ enum mmc_busy_cmd {
 	MMC_BUSY_IO,
 };

+struct mmc_busy_data {
+	struct mmc_card *card;
+	bool retry_crc_err;
+	bool aggregate_err_flags;
+	enum mmc_busy_cmd busy_cmd;
+	u32 *status;
+};
+
 struct mmc_host;
 struct mmc_card;
 struct mmc_command;
@@ -41,6 +49,7 @@ int mmc_can_ext_csd(struct mmc_card *card);
 int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
 bool mmc_prepare_busy_cmd(struct mmc_host *host, struct mmc_command *cmd,
 			  unsigned int timeout_ms);
+int mmc_busy_cb(void *cb_data, bool *busy);
 int __mmc_poll_for_busy(struct mmc_host *host, unsigned int period_us,
 			unsigned int timeout_ms,
 			int (*busy_cb)(void *cb_data, bool *busy),
--
2.37.3

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

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

[Avri Altman]

> 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>
Acked-by: Avri Altman <avri.altman@wdc.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 */
> +	if (idata->rpmb || idata->ic.write_flag || r1b_resp) {
AFAIK write_flag  and r1b_resp are set together (in mmc-utils that is)?
As for rpmb read operations you were pondering about -
the rpmb read-counter is one example, because you use it to sign write operations.
So restoring all rpmb operations is in place - all should be monitored.

> +		struct mmc_busy_data cb_data;
Maybe use designated initializing?

Thanks,
Avri
> +
> +		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;
>  }
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 3b3adbddf664..15d8b806c670 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -54,11 +54,6 @@ static const u8 tuning_blk_pattern_8bit[] = {
>  	0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
>  };
> 
> -struct mmc_busy_data {
> -	struct mmc_card *card;
> -	bool retry_crc_err;
> -	enum mmc_busy_cmd busy_cmd;
> -};
> 
>  struct mmc_op_cond_busy_data {
>  	struct mmc_host *host;
> @@ -457,14 +452,15 @@ int mmc_switch_status(struct mmc_card *card, bool
> crc_err_fatal)
>  	return mmc_switch_status_error(card->host, status);
>  }
> 
> -static int mmc_busy_cb(void *cb_data, bool *busy)
> +int mmc_busy_cb(void *cb_data, bool *busy)
>  {
>  	struct mmc_busy_data *data = cb_data;
>  	struct mmc_host *host = data->card->host;
>  	u32 status = 0;
>  	int err;
> 
> -	if (data->busy_cmd != MMC_BUSY_IO && host->ops->card_busy) {
> +	if (data->busy_cmd != MMC_BUSY_IO && host->ops->card_busy &&
> +			!data->aggregate_err_flags) {
>  		*busy = host->ops->card_busy(host);
>  		return 0;
>  	}
> @@ -477,6 +473,9 @@ static int mmc_busy_cb(void *cb_data, bool *busy)
>  	if (err)
>  		return err;
> 
> +	if (data->aggregate_err_flags)
> +		*data->status = R1_STATUS(*data->status) | status;
> +
>  	switch (data->busy_cmd) {
>  	case MMC_BUSY_CMD6:
>  		err = mmc_switch_status_error(host, status);
> @@ -549,6 +548,7 @@ int mmc_poll_for_busy(struct mmc_card *card,
> unsigned int timeout_ms,
> 
>  	cb_data.card = card;
>  	cb_data.retry_crc_err = retry_crc_err;
> +	cb_data.aggregate_err_flags = false;
>  	cb_data.busy_cmd = busy_cmd;
> 
>  	return __mmc_poll_for_busy(host, 0, timeout_ms, &mmc_busy_cb,
> &cb_data);
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 09ffbc00908b..a57751b83f19 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -18,6 +18,14 @@ enum mmc_busy_cmd {
>  	MMC_BUSY_IO,
>  };
> 
> +struct mmc_busy_data {
> +	struct mmc_card *card;
> +	bool retry_crc_err;
> +	bool aggregate_err_flags;
> +	enum mmc_busy_cmd busy_cmd;
> +	u32 *status;
> +};
> +
>  struct mmc_host;
>  struct mmc_card;
>  struct mmc_command;
> @@ -41,6 +49,7 @@ int mmc_can_ext_csd(struct mmc_card *card);
>  int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
>  bool mmc_prepare_busy_cmd(struct mmc_host *host, struct mmc_command
> *cmd,
>  			  unsigned int timeout_ms);
> +int mmc_busy_cb(void *cb_data, bool *busy);
>  int __mmc_poll_for_busy(struct mmc_host *host, unsigned int period_us,
>  			unsigned int timeout_ms,
>  			int (*busy_cb)(void *cb_data, bool *busy),
> --
> 2.37.3

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

[Christian Loehle]

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




> -----Original Message-----
> From: Avri Altman <Avri.Altman@wdc.com>
> Sent: Wednesday, June 21, 2023 9:31 AM
> To: Christian Loehle <CLoehle@hyperstone.com>; linux-mmc@vger.kernel.org;
> linux-kernel@vger.kernel.org; Ulf Hansson <ulf.hansson@linaro.org>; Adrian
> Hunter <adrian.hunter@intel.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!
>
> > 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>
> Acked-by: Avri Altman <avri.altman@wdc.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 */
> > +     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 #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 10302 bytes --]

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

[Ulf Hansson]

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.

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.

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

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

[Christian Loehle]

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

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

[Ulf Hansson]

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