| 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
| ||
|
Date: Tue, 16 May 2023 15:37:29 +0000
From: Christian Loehle <CLoehle@...erstone.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
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
-----Original Message-----
From: Ulf Hansson <ulf.hansson@...aro.org>
Sent: Monday, May 15, 2023 3:56 PM
To: Christian Loehle <CLoehle@...erstone.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>; linux-mmc@...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
>>
>> 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.
I'll retest, it was mostly about the new bit with e.g. a switch command to be more
explicit, it doesn't actually break anything to enter, but to emphasize
that write_flag != 0 does not imply idata->buf, which is counter-intuitive.
Will rework in v2
>
>> 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?
>
Strictly speaking no, i chose it for three reasons:
a) it's more flexible, don't have to hardcode some operations
b) it doesn't change current userspace interface, acts like a versioning of the ioctl interface.
c) it's easy to check if userspace makes use of it, one could even think of adding a WARN_ON if e.g.
sanitize is called without MMC_AGGREGATE_PROG_ERRORS as it might create a false sense of security -> is insecure.
>> 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.
It doesn't affect mmc-utils and if we consider it the only client that's fair, but with thoughts about removing the flag
it felt a bit off tbh. The status and ready for data field will of course be non-sense. (I think I will overwrite the status fields
with the last polled status and only aggregate the error flags, to at least accomodate for that.)
>> + */
>> + 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.
Makes sense.
>> /* 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 */
So summarizing I will change:
- use __mmc_poll_for_busy with custom mmc_blk_busy_cb to aggregate error flags
- set non-error flags to last polled CMD13.
- put everything in resp[0]
- enable for all with write_flag != 0 or R1b response
- (use early return for spi write case)
Thanks to the both of you for taking a look at it.
Regards,
Christian
Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
Powered by blists - more mailing lists