| 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: <CAPDyKFp5zi=KEgq7P4E7y5u7owM+C2991sBs+8QVGGCN8C+89A@mail.gmail.com>
Date: Mon, 15 May 2023 15:56:12 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Christian Löhle <CLoehle@...erstone.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Avri Altman <avri.altman@....com>
Subject: Re: [PATCH 3/3] mmc: block: ioctl: Add error aggregation flag
On Wed, 5 Apr 2023 at 13:57, Christian Löhle <CLoehle@...erstone.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@...erstone.com>
> ---
> drivers/mmc/core/block.c | 34 ++++++++++++++++++++++++++++++++--
> include/uapi/linux/mmc/ioctl.h | 2 ++
> 2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 35ff7101cbb1..386a508bd720 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -457,7 +457,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
> sizeof(ic->response)))
> return -EFAULT;
>
> - if (!idata->ic.write_flag) {
> + if (!idata->ic.write_flag && idata->buf) {
If needed, this looks like it should be a separate change.
> if (copy_to_user((void __user *)(unsigned long)ic->data_ptr,
> idata->buf, idata->buf_bytes))
> return -EFAULT;
> @@ -610,13 +610,43 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
> 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)
> + if ((card->host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp &&
> + !(idata->ic.write_flag & MMC_AGGREGATE_PROG_ERRORS))
Do we really need a new flag for this? Can't we just collect the error
code always for writes? Or collect the errors based upon a selection
of commands?
> return 0;
>
> if (mmc_host_is_spi(card->host)) {
> if (idata->ic.write_flag)
> err = mmc_spi_err_check(card);
> }
> + /*
> + * We want to receive a meaningful R1 response for errors of detection
> + * type X, which are only set after the card has executed the command.
> + * In that case poll CMD13 until PROG is left and return the
> + * accumulated error bits.
> + * If we're lucky host controller has busy detection for R1B and
> + * this is just a single CMD13.
> + * We can abuse resp[1] as the post-PROG R1 here, as all commands
> + * that go through PRG have an R1 response and therefore only
> + * use resp[0].
Hmm, for these cases, is the resp[0] containing any interesting
information? Probably not, right?
In that case, why not override the resp[0], this should make the
behaviour more consistent from user space point of view.
> + */
> + else if (idata->ic.write_flag & MMC_AGGREGATE_PROG_ERRORS) {
> + unsigned long timeout = jiffies +
> + msecs_to_jiffies(busy_timeout_ms);
> + bool done = false;
> + unsigned long delay_ms = 1;
> + u32 status;
> +
> + do {
> + done = time_after(jiffies, timeout);
> + msleep(delay_ms++);
> + err = __mmc_send_status(card, &status, 1);
> + if (err)
> + return err;
> + idata->ic.response[1] |= status;
> + } while (R1_CURRENT_STATE(status) != R1_STATE_TRAN && !done);
> + if (done)
> + return -ETIMEDOUT;
> + }
We have moved away from open-coding polling loops. Let's not introduce
a new one. In fact, it looks a bit like we could re-use the
mmc_blk_busy_cb() for this, as it seems to be collecting the error
codes too.
In any case, let's at least make use of __mmc_poll_for_busy() to avoid
the open-coding.
> /* Ensure RPMB/R1B command has completed by polling with CMD13. */
> else if (idata->rpmb || r1b_resp)
> err = mmc_poll_for_busy(card, busy_timeout_ms, false,
> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
> index b2ff7f5be87b..a9d84ae8d57d 100644
> --- a/include/uapi/linux/mmc/ioctl.h
> +++ b/include/uapi/linux/mmc/ioctl.h
> @@ -8,9 +8,11 @@
> struct mmc_ioc_cmd {
> /*
> * Direction of data: nonzero = write, zero = read.
> + * Bit 30 selects error aggregation post-PROG state.
> * Bit 31 selects 'Reliable Write' for RPMB.
> */
> int write_flag;
> +#define MMC_AGGREGATE_PROG_ERRORS (1 << 30)
> #define MMC_RPMB_RELIABLE_WRITE (1 << 31)
>
> /* Application-specific command. true = precede with CMD55 */
Kind regards
Uffe
Powered by blists - more mailing lists