| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BL0PR04MB65647F33DC7CC1D2903E5A66FC5DA@BL0PR04MB6564.namprd04.prod.outlook.com>
Date: Wed, 21 Jun 2023 07:31:11 +0000
From: Avri Altman <Avri.Altman@....com>
To: Christian Loehle <CLoehle@...erstone.com>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ulf Hansson <ulf.hansson@...aro.org>,
Adrian Hunter <adrian.hunter@...el.com>
Subject: RE: [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@...erstone.com>
Acked-by: Avri Altman <avri.altman@....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
Powered by blists - more mailing lists