| Message ID | 043d49e37e254eb8aa8a2c5fc56a028b@hyperstone.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, 8 May 2023 at 13:46, Christian Loehle <CLoehle@hyperstone.com> wrote: > > Did anyone find the time to look at this yet? > Any comments? Hi Christian, It's been a busy time, my apologies for the delay. I will soon come to review this! Kind regards Uffe > > -----Original Message----- > From: Christian Löhle > Sent: Wednesday, April 5, 2023 1:58 PM > To: Ulf Hansson <ulf.hansson@linaro.org>; Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Avri Altman <avri.altman@wdc.com> > Subject: [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@hyperstone.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 (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)) > > 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]. > > + */ > > + 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; > > + } > > /* 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 */ > > -- > > 2.37.3 > > > > > > Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz > Managing Director: Dr. Jan Peter Berns. > Commercial register of local courts: Freiburg HRB381782 >
On Wed, 5 Apr 2023 at 13:57, Christian Löhle <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 | 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
> > + */ > > + 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. Or maybe use read_poll_timeout()? Thanks, Avri
-----Original Message----- From: Ulf Hansson <ulf.hansson@linaro.org> Sent: Monday, May 15, 2023 3:56 PM To: Christian Loehle <CLoehle@hyperstone.com> Cc: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Avri Altman <avri.altman@wdc.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@hyperstone.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
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 (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)) 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]. + */ + 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; + } /* 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 */
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 | 34 ++++++++++++++++++++++++++++++++-- include/uapi/linux/mmc/ioctl.h | 2 ++ 2 files changed, 34 insertions(+), 2 deletions(-)