| Message ID | 20231129092535.3278-1-avri.altman@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] mmc: core: Use mrq.sbc in close-ended ffu | expand |
Hi, On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> wrote: > > > > Field Firmware Update (ffu) may use close-ended or open ended sequence. > > Each such sequence is comprised of a write commands enclosed between 2 > > switch commands - to and from ffu mode. So for the close-ended case, it > > will be: cmd6->cmd23-cmd25-cmd6. > > > > Some host controllers however, get confused when multi-block rw is sent > > without sbc, and may generate auto-cmd12 which breaks the ffu sequence. > > I encountered this issue while testing fwupd (github.com/fwupd/fwupd) > > on HP Chromebook x2, a qualcomm based QC-7c, code name - strongbad. > > > > Instead of a quirk, or hooking the request function of the msm ops, > > it would be better to fix the ioctl handling and make it use mrq.sbc > > instead of issuing SET_BLOCK_COUNT separately. > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > Applied for next (to get it tested a bit more) and by adding a stable > tag, thanks! This change is causing RPMB breakage in 6.6.13 and also 6.6.20. rockpi4b and synquacer arm64 boards with u-boot, optee 4.1.0 and firmware TPM (fTPM) fail to access RPMB via kernel and tee-supplicant 4.1.0. More details in https://bugzilla.kernel.org/show_bug.cgi?id=218587 I've tried to identify what exactly is going wrong but failed so far. Reverting this changes is the only working solution so far. This also triggered a kernel crash on error path https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now fixed/queued. If you have any hints how to debug this further or patches to try, I'd be happy to try those out. Cheers, -Mikko > Kind regards > Uffe > > > > --- > > > > Changelog: > > v3--v4: > > check sbc.error as well > > v2--v3: > > Adopt Adrian's proposal > > v1--v2: > > remove redundant reference of reliable write > > --- > > drivers/mmc/core/block.c | 46 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > > index f9a5cffa64b1..892e74e611a0 100644 > > --- a/drivers/mmc/core/block.c > > +++ b/drivers/mmc/core/block.c > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > struct mmc_ioc_cmd ic; > > unsigned char *buf; > > u64 buf_bytes; > > + unsigned int flags; > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > + > > struct mmc_rpmb_data *rpmb; > > }; > > > > @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, > > } > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > - struct mmc_blk_ioc_data *idata) > > + struct mmc_blk_ioc_data **idatas, int i) > > { > > struct mmc_command cmd = {}, sbc = {}; > > struct mmc_data data = {}; > > @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > unsigned int busy_timeout_ms; > > int err; > > unsigned int target_part; > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > if (!card || !md || !idata) > > return -EINVAL; > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > + return 0; > > + > > + if (idata->flags & MMC_BLK_IOC_SBC) > > + prev_idata = idatas[i - 1]; > > + > > /* > > * The RPMB accesses comes in from the character device, so we > > * need to target these explicitly. Else we just target the > > @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > return err; > > } > > > > - if (idata->rpmb) { > > + if (idata->rpmb || prev_idata) { > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > /* > > * We don't do any blockcount validation because the max size > > @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > * 'Reliable Write' bit here. > > */ > > sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31)); > > + if (prev_idata) > > + sbc.arg = prev_idata->ic.arg; > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > mrq.sbc = &sbc; > > } > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > > mmc_wait_for_req(card->host, &mrq); > > memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); > > > > + if (prev_idata) { > > + memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp)); > > + if (sbc.error) { > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > + __func__, sbc.error); > > + return sbc.error; > > + } > > + } > > + > > if (cmd.error) { > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > __func__, cmd.error); > > @@ -1032,6 +1055,20 @@ static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > md->reset_done &= ~type; > > } > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) > > +{ > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > + int i; > > + > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT && > > + mmc_op_multi(idata[i]->ic.opcode)) { > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > + } > > + } > > +} > > + > > /* > > * The non-block commands come back from the block layer after it queued it and > > * processed it with all other requests and then they get issued in this > > @@ -1059,11 +1096,14 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req) > > if (ret) > > break; > > } > > + > > + mmc_blk_check_sbc(mq_rq); > > + > > fallthrough; > > case MMC_DRV_OP_IOCTL_RPMB: > > idata = mq_rq->drv_op_data; > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > + ret = __mmc_blk_ioctl_cmd(card, md, idata, i); > > if (ret) > > break; > > } > > -- > > 2.42.0 > >
> On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> wrote: > > > > > > Field Firmware Update (ffu) may use close-ended or open ended sequence. > > > Each such sequence is comprised of a write commands enclosed between > > > 2 switch commands - to and from ffu mode. So for the close-ended > > > case, it will be: cmd6->cmd23-cmd25-cmd6. > > > > > > Some host controllers however, get confused when multi-block rw is > > > sent without sbc, and may generate auto-cmd12 which breaks the ffu > sequence. > > > I encountered this issue while testing fwupd > > > (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm based QC- > 7c, code name - strongbad. > > > > > > Instead of a quirk, or hooking the request function of the msm ops, > > > it would be better to fix the ioctl handling and make it use mrq.sbc > > > instead of issuing SET_BLOCK_COUNT separately. > > > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > Applied for next (to get it tested a bit more) and by adding a stable > > tag, thanks! > > This change is causing RPMB breakage in 6.6.13 and also 6.6.20. rockpi4b and > synquacer arm64 boards with u-boot, optee 4.1.0 and firmware TPM (fTPM) fail > to access RPMB via kernel and tee-supplicant 4.1.0. > > More details in https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > I've tried to identify what exactly is going wrong but failed so far. Reverting this > changes is the only working solution so far. This also triggered a kernel crash on > error path https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now > fixed/queued. > > If you have any hints how to debug this further or patches to try, I'd be happy to > try those out. I don't know nothing about tpm nor the ftpm. The above patch is scanning command sequences sent via MMC_IOC_MULTI_CMD, looking for pairs of CMD23->CMD25 or CMD23->CMD18, drops the CMD23 and build one instead in __mmc_blk_ioctl_cmd as the mrq.sbc. AFAIK user-space utils, e.g. mmc-utils count on the mmc driver to provide SBC when accessing rpmb, so their multi-ioctl sequences does not include CMD23, hence does not affected by this code. Looking in the strace included, I tried to find where MMC_IOC_MULTI_CMD is sent. There are 8 such ioctl calls. I guess the tee supplicant sources are unavailable, But it looks like 2 simultaneous threads are trying to access the rpmb partition - which is forbidden. Opening /dev/mmcblk0rpmb from user space is exclusive, so I am not sure how even this is possible. I can try and help you debug this - you can contact me offline. Thanks, Avri > > Cheers, > > -Mikko > > > Kind regards > > Uffe > > > > > > > --- > > > > > > Changelog: > > > v3--v4: > > > check sbc.error as well > > > v2--v3: > > > Adopt Adrian's proposal > > > v1--v2: > > > remove redundant reference of reliable write > > > --- > > > drivers/mmc/core/block.c | 46 > > > +++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > > > index f9a5cffa64b1..892e74e611a0 100644 > > > --- a/drivers/mmc/core/block.c > > > +++ b/drivers/mmc/core/block.c > > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > > struct mmc_ioc_cmd ic; > > > unsigned char *buf; > > > u64 buf_bytes; > > > + unsigned int flags; > > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > > + > > > struct mmc_rpmb_data *rpmb; > > > }; > > > > > > @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct > > > mmc_ioc_cmd __user *ic_ptr, } > > > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct > mmc_blk_data *md, > > > - struct mmc_blk_ioc_data *idata) > > > + struct mmc_blk_ioc_data **idatas, int > > > + i) > > > { > > > struct mmc_command cmd = {}, sbc = {}; > > > struct mmc_data data = {}; > > > @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct > mmc_card *card, struct mmc_blk_data *md, > > > unsigned int busy_timeout_ms; > > > int err; > > > unsigned int target_part; > > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > > > if (!card || !md || !idata) > > > return -EINVAL; > > > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > > + return 0; > > > + > > > + if (idata->flags & MMC_BLK_IOC_SBC) > > > + prev_idata = idatas[i - 1]; > > > + > > > /* > > > * The RPMB accesses comes in from the character device, so we > > > * need to target these explicitly. Else we just target the > > > @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > *card, struct mmc_blk_data *md, > > > return err; > > > } > > > > > > - if (idata->rpmb) { > > > + if (idata->rpmb || prev_idata) { > > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > > /* > > > * We don't do any blockcount validation because the > > > max size @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct > mmc_card *card, struct mmc_blk_data *md, > > > * 'Reliable Write' bit here. > > > */ > > > sbc.arg = data.blocks | (idata->ic.write_flag & > > > BIT(31)); > > > + if (prev_idata) > > > + sbc.arg = prev_idata->ic.arg; > > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > > mrq.sbc = &sbc; > > > } > > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > *card, struct mmc_blk_data *md, > > > mmc_wait_for_req(card->host, &mrq); > > > memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); > > > > > > + if (prev_idata) { > > > + memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp)); > > > + if (sbc.error) { > > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > > + __func__, sbc.error); > > > + return sbc.error; > > > + } > > > + } > > > + > > > if (cmd.error) { > > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > > __func__, > > > cmd.error); @@ -1032,6 +1055,20 @@ static inline void > mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > > md->reset_done &= ~type; > > > } > > > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) { > > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > > + int i; > > > + > > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT && > > > + mmc_op_multi(idata[i]->ic.opcode)) { > > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > > + } > > > + } > > > +} > > > + > > > /* > > > * The non-block commands come back from the block layer after it queued > it and > > > * processed it with all other requests and then they get issued in > > > this @@ -1059,11 +1096,14 @@ static void mmc_blk_issue_drv_op(struct > mmc_queue *mq, struct request *req) > > > if (ret) > > > break; > > > } > > > + > > > + mmc_blk_check_sbc(mq_rq); > > > + > > > fallthrough; > > > case MMC_DRV_OP_IOCTL_RPMB: > > > idata = mq_rq->drv_op_data; > > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > > + ret = __mmc_blk_ioctl_cmd(card, md, idata, > > > + i); > > > if (ret) > > > break; > > > } > > > -- > > > 2.42.0 > > >
Hi, Adding Jens from OP-TEE. On Mon, Mar 11, 2024 at 02:55:01PM +0000, Avri Altman wrote: > > On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > > > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> wrote: > > > > > > > > Field Firmware Update (ffu) may use close-ended or open ended sequence. > > > > Each such sequence is comprised of a write commands enclosed between > > > > 2 switch commands - to and from ffu mode. So for the close-ended > > > > case, it will be: cmd6->cmd23-cmd25-cmd6. > > > > > > > > Some host controllers however, get confused when multi-block rw is > > > > sent without sbc, and may generate auto-cmd12 which breaks the ffu > > sequence. > > > > I encountered this issue while testing fwupd > > > > (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm based QC- > > 7c, code name - strongbad. > > > > > > > > Instead of a quirk, or hooking the request function of the msm ops, > > > > it would be better to fix the ioctl handling and make it use mrq.sbc > > > > instead of issuing SET_BLOCK_COUNT separately. > > > > > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > > > Applied for next (to get it tested a bit more) and by adding a stable > > > tag, thanks! > > > > This change is causing RPMB breakage in 6.6.13 and also 6.6.20. rockpi4b and > > synquacer arm64 boards with u-boot, optee 4.1.0 and firmware TPM (fTPM) fail > > to access RPMB via kernel and tee-supplicant 4.1.0. > > > > More details in https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > > > I've tried to identify what exactly is going wrong but failed so far. Reverting this > > changes is the only working solution so far. This also triggered a kernel crash on > > error path https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now > > fixed/queued. > > > > If you have any hints how to debug this further or patches to try, I'd be happy to > > try those out. > I don't know nothing about tpm nor the ftpm. > The above patch is scanning command sequences sent via MMC_IOC_MULTI_CMD, > looking for pairs of CMD23->CMD25 or CMD23->CMD18, > drops the CMD23 and build one instead in __mmc_blk_ioctl_cmd as the mrq.sbc. > AFAIK user-space utils, e.g. mmc-utils count on the mmc driver to provide SBC when accessing rpmb, > so their multi-ioctl sequences does not include CMD23, hence does not affected by this code. > > Looking in the strace included, I tried to find where MMC_IOC_MULTI_CMD is sent. > There are 8 such ioctl calls. > I guess the tee supplicant sources are unavailable, > But it looks like 2 simultaneous threads are trying to access the rpmb partition - which is forbidden. > Opening /dev/mmcblk0rpmb from user space is exclusive, so I am not sure how even this is possible. tee-supplicant sources are available from https://github.com/OP-TEE/optee_client and specifically https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/rpmb.c#L893 for MMC_IOC_MULTI_CMD. Interesting if there are two threads trying to access RPMB at the same time. Any comments here from Jens? I would have expected kernel to keep RPMB access exclusive for a single user. Cheers, -Mikko > I can try and help you debug this - you can contact me offline. > > Thanks, > Avri > > > > > Cheers, > > > > -Mikko > > > > > Kind regards > > > Uffe > > > > > > > > > > --- > > > > > > > > Changelog: > > > > v3--v4: > > > > check sbc.error as well > > > > v2--v3: > > > > Adopt Adrian's proposal > > > > v1--v2: > > > > remove redundant reference of reliable write > > > > --- > > > > drivers/mmc/core/block.c | 46 > > > > +++++++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > > > > index f9a5cffa64b1..892e74e611a0 100644 > > > > --- a/drivers/mmc/core/block.c > > > > +++ b/drivers/mmc/core/block.c > > > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > > > struct mmc_ioc_cmd ic; > > > > unsigned char *buf; > > > > u64 buf_bytes; > > > > + unsigned int flags; > > > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > > > + > > > > struct mmc_rpmb_data *rpmb; > > > > }; > > > > > > > > @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct > > > > mmc_ioc_cmd __user *ic_ptr, } > > > > > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct > > mmc_blk_data *md, > > > > - struct mmc_blk_ioc_data *idata) > > > > + struct mmc_blk_ioc_data **idatas, int > > > > + i) > > > > { > > > > struct mmc_command cmd = {}, sbc = {}; > > > > struct mmc_data data = {}; > > > > @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct > > mmc_card *card, struct mmc_blk_data *md, > > > > unsigned int busy_timeout_ms; > > > > int err; > > > > unsigned int target_part; > > > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > > > > > if (!card || !md || !idata) > > > > return -EINVAL; > > > > > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > > > + return 0; > > > > + > > > > + if (idata->flags & MMC_BLK_IOC_SBC) > > > > + prev_idata = idatas[i - 1]; > > > > + > > > > /* > > > > * The RPMB accesses comes in from the character device, so we > > > > * need to target these explicitly. Else we just target the > > > > @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > > *card, struct mmc_blk_data *md, > > > > return err; > > > > } > > > > > > > > - if (idata->rpmb) { > > > > + if (idata->rpmb || prev_idata) { > > > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > > > /* > > > > * We don't do any blockcount validation because the > > > > max size @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct > > mmc_card *card, struct mmc_blk_data *md, > > > > * 'Reliable Write' bit here. > > > > */ > > > > sbc.arg = data.blocks | (idata->ic.write_flag & > > > > BIT(31)); > > > > + if (prev_idata) > > > > + sbc.arg = prev_idata->ic.arg; > > > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > > > mrq.sbc = &sbc; > > > > } > > > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > > *card, struct mmc_blk_data *md, > > > > mmc_wait_for_req(card->host, &mrq); > > > > memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); > > > > > > > > + if (prev_idata) { > > > > + memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp)); > > > > + if (sbc.error) { > > > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > > > + __func__, sbc.error); > > > > + return sbc.error; > > > > + } > > > > + } > > > > + > > > > if (cmd.error) { > > > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > > > __func__, > > > > cmd.error); @@ -1032,6 +1055,20 @@ static inline void > > mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > > > md->reset_done &= ~type; > > > > } > > > > > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) { > > > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > > > + int i; > > > > + > > > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT && > > > > + mmc_op_multi(idata[i]->ic.opcode)) { > > > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > > > + } > > > > + } > > > > +} > > > > + > > > > /* > > > > * The non-block commands come back from the block layer after it queued > > it and > > > > * processed it with all other requests and then they get issued in > > > > this @@ -1059,11 +1096,14 @@ static void mmc_blk_issue_drv_op(struct > > mmc_queue *mq, struct request *req) > > > > if (ret) > > > > break; > > > > } > > > > + > > > > + mmc_blk_check_sbc(mq_rq); > > > > + > > > > fallthrough; > > > > case MMC_DRV_OP_IOCTL_RPMB: > > > > idata = mq_rq->drv_op_data; > > > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > > > + ret = __mmc_blk_ioctl_cmd(card, md, idata, > > > > + i); > > > > if (ret) > > > > break; > > > > } > > > > -- > > > > 2.42.0 > > > >
On Mon, Mar 11, 2024 at 4:08 PM Mikko Rapeli <mikko.rapeli@linaro.org> wrote: > > Hi, > > Adding Jens from OP-TEE. > > On Mon, Mar 11, 2024 at 02:55:01PM +0000, Avri Altman wrote: > > > On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > > > > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> wrote: > > > > > > > > > > Field Firmware Update (ffu) may use close-ended or open ended sequence. > > > > > Each such sequence is comprised of a write commands enclosed between > > > > > 2 switch commands - to and from ffu mode. So for the close-ended > > > > > case, it will be: cmd6->cmd23-cmd25-cmd6. > > > > > > > > > > Some host controllers however, get confused when multi-block rw is > > > > > sent without sbc, and may generate auto-cmd12 which breaks the ffu > > > sequence. > > > > > I encountered this issue while testing fwupd > > > > > (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm based QC- > > > 7c, code name - strongbad. > > > > > > > > > > Instead of a quirk, or hooking the request function of the msm ops, > > > > > it would be better to fix the ioctl handling and make it use mrq.sbc > > > > > instead of issuing SET_BLOCK_COUNT separately. > > > > > > > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > > > > > Applied for next (to get it tested a bit more) and by adding a stable > > > > tag, thanks! > > > > > > This change is causing RPMB breakage in 6.6.13 and also 6.6.20. rockpi4b and > > > synquacer arm64 boards with u-boot, optee 4.1.0 and firmware TPM (fTPM) fail > > > to access RPMB via kernel and tee-supplicant 4.1.0. > > > > > > More details in https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > > > > > I've tried to identify what exactly is going wrong but failed so far. Reverting this > > > changes is the only working solution so far. This also triggered a kernel crash on > > > error path https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now > > > fixed/queued. > > > > > > If you have any hints how to debug this further or patches to try, I'd be happy to > > > try those out. > > I don't know nothing about tpm nor the ftpm. > > The above patch is scanning command sequences sent via MMC_IOC_MULTI_CMD, > > looking for pairs of CMD23->CMD25 or CMD23->CMD18, > > drops the CMD23 and build one instead in __mmc_blk_ioctl_cmd as the mrq.sbc. > > AFAIK user-space utils, e.g. mmc-utils count on the mmc driver to provide SBC when accessing rpmb, > > so their multi-ioctl sequences does not include CMD23, hence does not affected by this code. > > > > Looking in the strace included, I tried to find where MMC_IOC_MULTI_CMD is sent. > > There are 8 such ioctl calls. > > I guess the tee supplicant sources are unavailable, > > But it looks like 2 simultaneous threads are trying to access the rpmb partition - which is forbidden. > > Opening /dev/mmcblk0rpmb from user space is exclusive, so I am not sure how even this is possible. > > tee-supplicant sources are available from https://github.com/OP-TEE/optee_client > and specifically https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/rpmb.c#L893 > for MMC_IOC_MULTI_CMD. > > Interesting if there are two threads trying to access RPMB at the same time. Any > comments here from Jens? I would have expected kernel to keep RPMB access > exclusive for a single user. tee-supplicant is multithreaded, but OP-TEE in the secure world has a global mutex to protect against concurrent access to RPMB. From the secure world point of view, it's needed to manage the write counter, but obviously, it has other side effects. See for instance rpmb_fs_open() at https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_rpmb_fs.c#L2953, it's the same for all the other functions accessing RPMB. Cheers, Jens > > Cheers, > > -Mikko > > > I can try and help you debug this - you can contact me offline. > > > > Thanks, > > Avri > > > > > > > > Cheers, > > > > > > -Mikko > > > > > > > Kind regards > > > > Uffe > > > > > > > > > > > > > --- > > > > > > > > > > Changelog: > > > > > v3--v4: > > > > > check sbc.error as well > > > > > v2--v3: > > > > > Adopt Adrian's proposal > > > > > v1--v2: > > > > > remove redundant reference of reliable write > > > > > --- > > > > > drivers/mmc/core/block.c | 46 > > > > > +++++++++++++++++++++++++++++++++++++--- > > > > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > > > > > index f9a5cffa64b1..892e74e611a0 100644 > > > > > --- a/drivers/mmc/core/block.c > > > > > +++ b/drivers/mmc/core/block.c > > > > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > > > > struct mmc_ioc_cmd ic; > > > > > unsigned char *buf; > > > > > u64 buf_bytes; > > > > > + unsigned int flags; > > > > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > > > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > > > > + > > > > > struct mmc_rpmb_data *rpmb; > > > > > }; > > > > > > > > > > @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct > > > > > mmc_ioc_cmd __user *ic_ptr, } > > > > > > > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct > > > mmc_blk_data *md, > > > > > - struct mmc_blk_ioc_data *idata) > > > > > + struct mmc_blk_ioc_data **idatas, int > > > > > + i) > > > > > { > > > > > struct mmc_command cmd = {}, sbc = {}; > > > > > struct mmc_data data = {}; > > > > > @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct > > > mmc_card *card, struct mmc_blk_data *md, > > > > > unsigned int busy_timeout_ms; > > > > > int err; > > > > > unsigned int target_part; > > > > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > > > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > > > > > > > if (!card || !md || !idata) > > > > > return -EINVAL; > > > > > > > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > > > > + return 0; > > > > > + > > > > > + if (idata->flags & MMC_BLK_IOC_SBC) > > > > > + prev_idata = idatas[i - 1]; > > > > > + > > > > > /* > > > > > * The RPMB accesses comes in from the character device, so we > > > > > * need to target these explicitly. Else we just target the > > > > > @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > > > *card, struct mmc_blk_data *md, > > > > > return err; > > > > > } > > > > > > > > > > - if (idata->rpmb) { > > > > > + if (idata->rpmb || prev_idata) { > > > > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > > > > /* > > > > > * We don't do any blockcount validation because the > > > > > max size @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct > > > mmc_card *card, struct mmc_blk_data *md, > > > > > * 'Reliable Write' bit here. > > > > > */ > > > > > sbc.arg = data.blocks | (idata->ic.write_flag & > > > > > BIT(31)); > > > > > + if (prev_idata) > > > > > + sbc.arg = prev_idata->ic.arg; > > > > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > > > > mrq.sbc = &sbc; > > > > > } > > > > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > > > *card, struct mmc_blk_data *md, > > > > > mmc_wait_for_req(card->host, &mrq); > > > > > memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); > > > > > > > > > > + if (prev_idata) { > > > > > + memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp)); > > > > > + if (sbc.error) { > > > > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > > > > + __func__, sbc.error); > > > > > + return sbc.error; > > > > > + } > > > > > + } > > > > > + > > > > > if (cmd.error) { > > > > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > > > > __func__, > > > > > cmd.error); @@ -1032,6 +1055,20 @@ static inline void > > > mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > > > > md->reset_done &= ~type; > > > > > } > > > > > > > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) { > > > > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > > > > + int i; > > > > > + > > > > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > > > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT && > > > > > + mmc_op_multi(idata[i]->ic.opcode)) { > > > > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > > > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > > > > + } > > > > > + } > > > > > +} > > > > > + > > > > > /* > > > > > * The non-block commands come back from the block layer after it queued > > > it and > > > > > * processed it with all other requests and then they get issued in > > > > > this @@ -1059,11 +1096,14 @@ static void mmc_blk_issue_drv_op(struct > > > mmc_queue *mq, struct request *req) > > > > > if (ret) > > > > > break; > > > > > } > > > > > + > > > > > + mmc_blk_check_sbc(mq_rq); > > > > > + > > > > > fallthrough; > > > > > case MMC_DRV_OP_IOCTL_RPMB: > > > > > idata = mq_rq->drv_op_data; > > > > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > > > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > > > > + ret = __mmc_blk_ioctl_cmd(card, md, idata, > > > > > + i); > > > > > if (ret) > > > > > break; > > > > > } > > > > > -- > > > > > 2.42.0 > > > > >
Hi, On Mon, Mar 11, 2024 at 04:19:54PM +0100, Jens Wiklander wrote: > On Mon, Mar 11, 2024 at 4:08 PM Mikko Rapeli <mikko.rapeli@linaro.org> wrote: > > > > Hi, > > > > Adding Jens from OP-TEE. > > > > On Mon, Mar 11, 2024 at 02:55:01PM +0000, Avri Altman wrote: > > > > On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > > > > > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> wrote: > > > > > > > > > > > > Field Firmware Update (ffu) may use close-ended or open ended sequence. > > > > > > Each such sequence is comprised of a write commands enclosed between > > > > > > 2 switch commands - to and from ffu mode. So for the close-ended > > > > > > case, it will be: cmd6->cmd23-cmd25-cmd6. > > > > > > > > > > > > Some host controllers however, get confused when multi-block rw is > > > > > > sent without sbc, and may generate auto-cmd12 which breaks the ffu > > > > sequence. > > > > > > I encountered this issue while testing fwupd > > > > > > (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm based QC- > > > > 7c, code name - strongbad. > > > > > > > > > > > > Instead of a quirk, or hooking the request function of the msm ops, > > > > > > it would be better to fix the ioctl handling and make it use mrq.sbc > > > > > > instead of issuing SET_BLOCK_COUNT separately. > > > > > > > > > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > > > > > > > Applied for next (to get it tested a bit more) and by adding a stable > > > > > tag, thanks! > > > > > > > > This change is causing RPMB breakage in 6.6.13 and also 6.6.20. rockpi4b and > > > > synquacer arm64 boards with u-boot, optee 4.1.0 and firmware TPM (fTPM) fail > > > > to access RPMB via kernel and tee-supplicant 4.1.0. > > > > > > > > More details in https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > > > > > > > I've tried to identify what exactly is going wrong but failed so far. Reverting this > > > > changes is the only working solution so far. This also triggered a kernel crash on > > > > error path https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now > > > > fixed/queued. > > > > > > > > If you have any hints how to debug this further or patches to try, I'd be happy to > > > > try those out. > > > I don't know nothing about tpm nor the ftpm. > > > The above patch is scanning command sequences sent via MMC_IOC_MULTI_CMD, > > > looking for pairs of CMD23->CMD25 or CMD23->CMD18, > > > drops the CMD23 and build one instead in __mmc_blk_ioctl_cmd as the mrq.sbc. > > > AFAIK user-space utils, e.g. mmc-utils count on the mmc driver to provide SBC when accessing rpmb, > > > so their multi-ioctl sequences does not include CMD23, hence does not affected by this code. > > > > > > Looking in the strace included, I tried to find where MMC_IOC_MULTI_CMD is sent. > > > There are 8 such ioctl calls. > > > I guess the tee supplicant sources are unavailable, > > > But it looks like 2 simultaneous threads are trying to access the rpmb partition - which is forbidden. > > > Opening /dev/mmcblk0rpmb from user space is exclusive, so I am not sure how even this is possible. > > > > tee-supplicant sources are available from https://github.com/OP-TEE/optee_client > > and specifically https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/rpmb.c#L893 > > for MMC_IOC_MULTI_CMD. > > > > Interesting if there are two threads trying to access RPMB at the same time. Any > > comments here from Jens? I would have expected kernel to keep RPMB access > > exclusive for a single user. > > tee-supplicant is multithreaded, but OP-TEE in the secure world has a > global mutex to protect against concurrent access to RPMB. From the > secure world point of view, it's needed to manage the write counter, > but obviously, it has other side effects. > See for instance rpmb_fs_open() at > https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_rpmb_fs.c#L2953, > it's the same for all the other functions accessing RPMB. Thanks. From what I gather from strace log output in https://bugzilla.kernel.org/show_bug.cgi?id=218587 is that indeed two threads send the commands but they share the same file descriptor number 6 and also use mutex to make access exclusive: [pid 162] 00:00:15 openat(AT_FDCWD, "/dev/mmcblk0rpmb", O_RDWR) = 6 [pid 162] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000b70) = 0 ... [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a084b0) = 0 ... [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a085d0) = 0 ... [pid 162] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000cc0) = 0 ... [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a086c0) = 0 ... [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a087e0) = 0 ... [pid 159] 00:00:16 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a08900) = 0 ... [pid 162] 00:00:16 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000e40) = 0 ... [pid 162] 00:00:16 ioctl(3, TEE_IOC_SUPPL_RECV, {buf_len=168, buf_ptr={func=0, num_params=5, params=[{attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE|TEE_IOCTL_PARAM_ATTR_META, a=0, b=0, c=0}, {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}]}E/TC:? 0 E/TC:? 0 TA panicked with code 0xffff0000 E/LD: Status of TA bc50d971-d4c9-42c4-82cb-343fb7f37896 E/LD: arch: aarch64 E/LD: region 0: va 0x40005000 pa 0x3061b000 size 0x002000 flags rw-s (ldelf) E/LD: region 1: va 0x40007000 pa 0x3061d000 size 0x008000 flags r-xs (ldelf) E/LD: region 2: va 0x4000f000 pa 0x30625000 size 0x001000 flags rw-s (ldelf) E/LD: region 3: va 0x40010000 pa 0x30626000 size 0x004000 flags rw-s (ldelf) E/LD: region 4: va 0x40014000 pa 0x3062a000 size 0x001000 flags r--s E/LD: region 5: va 0x40015000 pa 0x306b2000 size 0x011000 flags rw-s (stack) E/LD: region 6: va 0x40026000 pa 0xe6bb4000 size 0x002000 flags rw-- (param) E/LD: region 7: va 0x4005b000 pa 0x00001000 size 0x068000 flags r-xs [0] E/LD: region 8: va 0x400c3000 pa 0x00069000 size 0x01f000 flags rw-s [0] E/LD: [0] bc50d971-d4c9-42c4-82cb-343fb7f37896 @ 0x4005b000 E/LD: Call stack: E/LD: 0x4009901c E/LD: 0x4005bb40 E/LD: 0x4005c1b8 E/LD: 0x4007af3c E/LD: 0x40093fc0 E/LD: 0x4005ca2c E/LD: 0x4009f1f4 E/LD: 0x40094170 [ 16.187823] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024 [ 16.188548] tpm tpm0: tpm_try_transmit: send(): error -53212 I presume with strace logging tee-supplicant runs a lot slower due to logging on serial console too so output is a bit mixed. Without strace the fTPM trusted application crash seems to happen immediately after "modprobe tpm_ftpm_tee". The TA side crash traces point to problems accessing data in secure storage, in RPMB via optee, kernel optee driver and userspace tee-supplicant. Cheers, -Mikko > Cheers, > Jens > > > > > Cheers, > > > > -Mikko > > > > > I can try and help you debug this - you can contact me offline. > > > > > > Thanks, > > > Avri > > > > > > > > > > > Cheers, > > > > > > > > -Mikko > > > > > > > > > Kind regards > > > > > Uffe > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > Changelog: > > > > > > v3--v4: > > > > > > check sbc.error as well > > > > > > v2--v3: > > > > > > Adopt Adrian's proposal > > > > > > v1--v2: > > > > > > remove redundant reference of reliable write > > > > > > --- > > > > > > drivers/mmc/core/block.c | 46 > > > > > > +++++++++++++++++++++++++++++++++++++--- > > > > > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > > > > > > index f9a5cffa64b1..892e74e611a0 100644 > > > > > > --- a/drivers/mmc/core/block.c > > > > > > +++ b/drivers/mmc/core/block.c > > > > > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > > > > > struct mmc_ioc_cmd ic; > > > > > > unsigned char *buf; > > > > > > u64 buf_bytes; > > > > > > + unsigned int flags; > > > > > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > > > > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > > > > > + > > > > > > struct mmc_rpmb_data *rpmb; > > > > > > }; > > > > > > > > > > > > @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct > > > > > > mmc_ioc_cmd __user *ic_ptr, } > > > > > > > > > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct > > > > mmc_blk_data *md, > > > > > > - struct mmc_blk_ioc_data *idata) > > > > > > + struct mmc_blk_ioc_data **idatas, int > > > > > > + i) > > > > > > { > > > > > > struct mmc_command cmd = {}, sbc = {}; > > > > > > struct mmc_data data = {}; > > > > > > @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct > > > > mmc_card *card, struct mmc_blk_data *md, > > > > > > unsigned int busy_timeout_ms; > > > > > > int err; > > > > > > unsigned int target_part; > > > > > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > > > > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > > > > > > > > > if (!card || !md || !idata) > > > > > > return -EINVAL; > > > > > > > > > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > > > > > + return 0; > > > > > > + > > > > > > + if (idata->flags & MMC_BLK_IOC_SBC) > > > > > > + prev_idata = idatas[i - 1]; > > > > > > + > > > > > > /* > > > > > > * The RPMB accesses comes in from the character device, so we > > > > > > * need to target these explicitly. Else we just target the > > > > > > @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > > > > *card, struct mmc_blk_data *md, > > > > > > return err; > > > > > > } > > > > > > > > > > > > - if (idata->rpmb) { > > > > > > + if (idata->rpmb || prev_idata) { > > > > > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > > > > > /* > > > > > > * We don't do any blockcount validation because the > > > > > > max size @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct > > > > mmc_card *card, struct mmc_blk_data *md, > > > > > > * 'Reliable Write' bit here. > > > > > > */ > > > > > > sbc.arg = data.blocks | (idata->ic.write_flag & > > > > > > BIT(31)); > > > > > > + if (prev_idata) > > > > > > + sbc.arg = prev_idata->ic.arg; > > > > > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > > > > > mrq.sbc = &sbc; > > > > > > } > > > > > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > > > > *card, struct mmc_blk_data *md, > > > > > > mmc_wait_for_req(card->host, &mrq); > > > > > > memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); > > > > > > > > > > > > + if (prev_idata) { > > > > > > + memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp)); > > > > > > + if (sbc.error) { > > > > > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > > > > > + __func__, sbc.error); > > > > > > + return sbc.error; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > if (cmd.error) { > > > > > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > > > > > __func__, > > > > > > cmd.error); @@ -1032,6 +1055,20 @@ static inline void > > > > mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > > > > > md->reset_done &= ~type; > > > > > > } > > > > > > > > > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) { > > > > > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > > > > > + int i; > > > > > > + > > > > > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > > > > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT && > > > > > > + mmc_op_multi(idata[i]->ic.opcode)) { > > > > > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > > > > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > > > > > + } > > > > > > + } > > > > > > +} > > > > > > + > > > > > > /* > > > > > > * The non-block commands come back from the block layer after it queued > > > > it and > > > > > > * processed it with all other requests and then they get issued in > > > > > > this @@ -1059,11 +1096,14 @@ static void mmc_blk_issue_drv_op(struct > > > > mmc_queue *mq, struct request *req) > > > > > > if (ret) > > > > > > break; > > > > > > } > > > > > > + > > > > > > + mmc_blk_check_sbc(mq_rq); > > > > > > + > > > > > > fallthrough; > > > > > > case MMC_DRV_OP_IOCTL_RPMB: > > > > > > idata = mq_rq->drv_op_data; > > > > > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > > > > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > > > > > + ret = __mmc_blk_ioctl_cmd(card, md, idata, > > > > > > + i); > > > > > > if (ret) > > > > > > break; > > > > > > } > > > > > > -- > > > > > > 2.42.0 > > > > > >
> > Hi, > > On Mon, Mar 11, 2024 at 04:19:54PM +0100, Jens Wiklander wrote: > > On Mon, Mar 11, 2024 at 4:08 PM Mikko Rapeli <mikko.rapeli@linaro.org> > wrote: > > > > > > Hi, > > > > > > Adding Jens from OP-TEE. > > > > > > On Mon, Mar 11, 2024 at 02:55:01PM +0000, Avri Altman wrote: > > > > > On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > > > > > > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> > wrote: > > > > > > > > > > > > > > Field Firmware Update (ffu) may use close-ended or open ended > sequence. > > > > > > > Each such sequence is comprised of a write commands enclosed > > > > > > > between > > > > > > > 2 switch commands - to and from ffu mode. So for the > > > > > > > close-ended case, it will be: cmd6->cmd23-cmd25-cmd6. > > > > > > > > > > > > > > Some host controllers however, get confused when multi-block > > > > > > > rw is sent without sbc, and may generate auto-cmd12 which > > > > > > > breaks the ffu > > > > > sequence. > > > > > > > I encountered this issue while testing fwupd > > > > > > > (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm > > > > > > > based QC- > > > > > 7c, code name - strongbad. > > > > > > > > > > > > > > Instead of a quirk, or hooking the request function of the > > > > > > > msm ops, it would be better to fix the ioctl handling and > > > > > > > make it use mrq.sbc instead of issuing SET_BLOCK_COUNT > separately. > > > > > > > > > > > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > > > > > > > > > Applied for next (to get it tested a bit more) and by adding a > > > > > > stable tag, thanks! > > > > > > > > > > This change is causing RPMB breakage in 6.6.13 and also 6.6.20. > > > > > rockpi4b and synquacer arm64 boards with u-boot, optee 4.1.0 and > > > > > firmware TPM (fTPM) fail to access RPMB via kernel and tee-supplicant > 4.1.0. > > > > > > > > > > More details in > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > > > > > > > > > I've tried to identify what exactly is going wrong but failed so > > > > > far. Reverting this changes is the only working solution so far. > > > > > This also triggered a kernel crash on error path > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now > fixed/queued. > > > > > > > > > > If you have any hints how to debug this further or patches to > > > > > try, I'd be happy to try those out. > > > > I don't know nothing about tpm nor the ftpm. > > > > The above patch is scanning command sequences sent via > > > > MMC_IOC_MULTI_CMD, looking for pairs of CMD23->CMD25 or > > > > CMD23->CMD18, drops the CMD23 and build one instead in > __mmc_blk_ioctl_cmd as the mrq.sbc. > > > > AFAIK user-space utils, e.g. mmc-utils count on the mmc driver to > > > > provide SBC when accessing rpmb, so their multi-ioctl sequences does not > include CMD23, hence does not affected by this code. > > > > > > > > Looking in the strace included, I tried to find where > MMC_IOC_MULTI_CMD is sent. > > > > There are 8 such ioctl calls. > > > > I guess the tee supplicant sources are unavailable, But it looks > > > > like 2 simultaneous threads are trying to access the rpmb partition - which > is forbidden. > > > > Opening /dev/mmcblk0rpmb from user space is exclusive, so I am not > sure how even this is possible. > > > > > > tee-supplicant sources are available from > > > https://github.com/OP-TEE/optee_client > > > and specifically > > > https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/sr > > > c/rpmb.c#L893 > > > for MMC_IOC_MULTI_CMD. I see no issue with this code. Moreover, Since it doesn't contains any CMD23 it shouldn't interact with the above patch. > > > > > > Interesting if there are two threads trying to access RPMB at the > > > same time. Any comments here from Jens? I would have expected kernel > > > to keep RPMB access exclusive for a single user. > > > > tee-supplicant is multithreaded, but OP-TEE in the secure world has a > > global mutex to protect against concurrent access to RPMB. From the > > secure world point of view, it's needed to manage the write counter, > > but obviously, it has other side effects. > > See for instance rpmb_fs_open() at > > https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_rpmb_fs.c# > > L2953, it's the same for all the other functions accessing RPMB. > > Thanks. From what I gather from strace log output in > https://bugzilla.kernel.org/show_bug.cgi?id=218587 > is that indeed two threads send the commands but they share the same file > descriptor number 6 and also use mutex to make access exclusive: > > [pid 162] 00:00:15 openat(AT_FDCWD, "/dev/mmcblk0rpmb", O_RDWR) = 6 > [pid 162] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000b70) = 0 > ... > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a084b0) = 0 > ... > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a085d0) = 0 > ... > [pid 162] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000cc0) = 0 > ... > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a086c0) = 0 > ... > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a087e0) = 0 > ... > [pid 159] 00:00:16 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a08900) = 0 > ... > [pid 162] 00:00:16 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000e40) = 0 > ... > [pid 162] 00:00:16 ioctl(3, TEE_IOC_SUPPL_RECV, {buf_len=168, > buf_ptr={func=0, num_params=5, > params=[{attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE|TEE_IOCTL_PARAM_ATT > R_META, a=0, b=0, c=0}, {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, > {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, > {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, > {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}]}E/TC:? 0 > E/TC:? 0 TA panicked with code 0xffff0000 > E/LD: Status of TA bc50d971-d4c9-42c4-82cb-343fb7f37896 > E/LD: arch: aarch64 > E/LD: region 0: va 0x40005000 pa 0x3061b000 size 0x002000 flags rw-s (ldelf) > E/LD: region 1: va 0x40007000 pa 0x3061d000 size 0x008000 flags r-xs (ldelf) > E/LD: region 2: va 0x4000f000 pa 0x30625000 size 0x001000 flags rw-s (ldelf) > E/LD: region 3: va 0x40010000 pa 0x30626000 size 0x004000 flags rw-s (ldelf) > E/LD: region 4: va 0x40014000 pa 0x3062a000 size 0x001000 flags r--s > E/LD: region 5: va 0x40015000 pa 0x306b2000 size 0x011000 flags rw-s (stack) > E/LD: region 6: va 0x40026000 pa 0xe6bb4000 size 0x002000 flags rw-- > (param) > E/LD: region 7: va 0x4005b000 pa 0x00001000 size 0x068000 flags r-xs [0] > E/LD: region 8: va 0x400c3000 pa 0x00069000 size 0x01f000 flags rw-s [0] > E/LD: [0] bc50d971-d4c9-42c4-82cb-343fb7f37896 @ 0x4005b000 > E/LD: Call stack: > E/LD: 0x4009901c > E/LD: 0x4005bb40 > E/LD: 0x4005c1b8 > E/LD: 0x4007af3c > E/LD: 0x40093fc0 > E/LD: 0x4005ca2c > E/LD: 0x4009f1f4 > E/LD: 0x40094170 > [ 16.187823] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke > error: 0xffff3024 > [ 16.188548] tpm tpm0: tpm_try_transmit: send(): error -53212 > > I presume with strace logging tee-supplicant runs a lot slower due to logging on > serial console too so output is a bit mixed. > > Without strace the fTPM trusted application crash seems to happen > immediately after "modprobe tpm_ftpm_tee". The TA side crash traces point to > problems accessing data in secure storage, in RPMB via optee, kernel optee > driver and userspace tee-supplicant. I noticed that the code is accessing RPMB as part of fat read? Is this something that can explain the above contention? Anyway, it might be helpful turning on the mmc dynamic debug - However Vanishingly unlikely, since the rpmb and the mmc device has 2 separate device nodes, Maybe there are 2 concurrent ioctls? AFAIK it doesn't get synchronized in the driver level. Thanks, Avri > > Cheers, > > -Mikko > > > Cheers, > > Jens > > > > > > > > Cheers, > > > > > > -Mikko > > > > > > > I can try and help you debug this - you can contact me offline. > > > > > > > > Thanks, > > > > Avri > > > > > > > > > > > > > > Cheers, > > > > > > > > > > -Mikko > > > > > > > > > > > Kind regards > > > > > > Uffe > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > Changelog: > > > > > > > v3--v4: > > > > > > > check sbc.error as well > > > > > > > v2--v3: > > > > > > > Adopt Adrian's proposal > > > > > > > v1--v2: > > > > > > > remove redundant reference of reliable write > > > > > > > --- > > > > > > > drivers/mmc/core/block.c | 46 > > > > > > > +++++++++++++++++++++++++++++++++++++--- > > > > > > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/mmc/core/block.c > > > > > > > b/drivers/mmc/core/block.c index f9a5cffa64b1..892e74e611a0 > > > > > > > 100644 > > > > > > > --- a/drivers/mmc/core/block.c > > > > > > > +++ b/drivers/mmc/core/block.c > > > > > > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > > > > > > struct mmc_ioc_cmd ic; > > > > > > > unsigned char *buf; > > > > > > > u64 buf_bytes; > > > > > > > + unsigned int flags; > > > > > > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > > > > > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > > > > > > + > > > > > > > struct mmc_rpmb_data *rpmb; }; > > > > > > > > > > > > > > @@ -465,7 +469,7 @@ static int > > > > > > > mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user > > > > > > > *ic_ptr, } > > > > > > > > > > > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, > > > > > > > struct > > > > > mmc_blk_data *md, > > > > > > > - struct mmc_blk_ioc_data *idata) > > > > > > > + struct mmc_blk_ioc_data > > > > > > > + **idatas, int > > > > > > > + i) > > > > > > > { > > > > > > > struct mmc_command cmd = {}, sbc = {}; > > > > > > > struct mmc_data data = {}; @@ -475,10 +479,18 @@ > > > > > > > static int __mmc_blk_ioctl_cmd(struct > > > > > mmc_card *card, struct mmc_blk_data *md, > > > > > > > unsigned int busy_timeout_ms; > > > > > > > int err; > > > > > > > unsigned int target_part; > > > > > > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > > > > > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > > > > > > > > > > > if (!card || !md || !idata) > > > > > > > return -EINVAL; > > > > > > > > > > > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > > > > > > + return 0; > > > > > > > + > > > > > > > + if (idata->flags & MMC_BLK_IOC_SBC) > > > > > > > + prev_idata = idatas[i - 1]; > > > > > > > + > > > > > > > /* > > > > > > > * The RPMB accesses comes in from the character device, so > we > > > > > > > * need to target these explicitly. Else we just > > > > > > > target the @@ -532,7 +544,7 @@ static int > > > > > > > __mmc_blk_ioctl_cmd(struct mmc_card > > > > > *card, struct mmc_blk_data *md, > > > > > > > return err; > > > > > > > } > > > > > > > > > > > > > > - if (idata->rpmb) { > > > > > > > + if (idata->rpmb || prev_idata) { > > > > > > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > > > > > > /* > > > > > > > * We don't do any blockcount validation > > > > > > > because the max size @@ -540,6 +552,8 @@ static int > > > > > > > __mmc_blk_ioctl_cmd(struct > > > > > mmc_card *card, struct mmc_blk_data *md, > > > > > > > * 'Reliable Write' bit here. > > > > > > > */ > > > > > > > sbc.arg = data.blocks | > > > > > > > (idata->ic.write_flag & BIT(31)); > > > > > > > + if (prev_idata) > > > > > > > + sbc.arg = prev_idata->ic.arg; > > > > > > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > > > > > > mrq.sbc = &sbc; > > > > > > > } > > > > > > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct > > > > > > > mmc_card > > > > > *card, struct mmc_blk_data *md, > > > > > > > mmc_wait_for_req(card->host, &mrq); > > > > > > > memcpy(&idata->ic.response, cmd.resp, > > > > > > > sizeof(cmd.resp)); > > > > > > > > > > > > > > + if (prev_idata) { > > > > > > > + memcpy(&prev_idata->ic.response, sbc.resp, > sizeof(sbc.resp)); > > > > > > > + if (sbc.error) { > > > > > > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > > > > > > + __func__, sbc.error); > > > > > > > + return sbc.error; > > > > > > > + } > > > > > > > + } > > > > > > > + > > > > > > > if (cmd.error) { > > > > > > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > > > > > > __func__, > > > > > > > cmd.error); @@ -1032,6 +1055,20 @@ static inline void > > > > > mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > > > > > > md->reset_done &= ~type; } > > > > > > > > > > > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) { > > > > > > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > > > > > > + int i; > > > > > > > + > > > > > > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > > > > > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT > && > > > > > > > + mmc_op_multi(idata[i]->ic.opcode)) { > > > > > > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > > > > > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > /* > > > > > > > * The non-block commands come back from the block layer > > > > > > > after it queued > > > > > it and > > > > > > > * processed it with all other requests and then they get > > > > > > > issued in this @@ -1059,11 +1096,14 @@ static void > > > > > > > mmc_blk_issue_drv_op(struct > > > > > mmc_queue *mq, struct request *req) > > > > > > > if (ret) > > > > > > > break; > > > > > > > } > > > > > > > + > > > > > > > + mmc_blk_check_sbc(mq_rq); > > > > > > > + > > > > > > > fallthrough; > > > > > > > case MMC_DRV_OP_IOCTL_RPMB: > > > > > > > idata = mq_rq->drv_op_data; > > > > > > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > > > > > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > > > > > > + ret = __mmc_blk_ioctl_cmd(card, md, > > > > > > > + idata, i); > > > > > > > if (ret) > > > > > > > break; > > > > > > > } > > > > > > > -- > > > > > > > 2.42.0 > > > > > > >
On Mon, Mar 11, 2024 at 8:25 PM Avri Altman <Avri.Altman@wdc.com> wrote: > > > > > Hi, > > > > On Mon, Mar 11, 2024 at 04:19:54PM +0100, Jens Wiklander wrote: > > > On Mon, Mar 11, 2024 at 4:08 PM Mikko Rapeli <mikko.rapeli@linaro.org> > > wrote: > > > > > > > > Hi, > > > > > > > > Adding Jens from OP-TEE. > > > > > > > > On Mon, Mar 11, 2024 at 02:55:01PM +0000, Avri Altman wrote: > > > > > > On Thu, Nov 30, 2023 at 11:36:10AM +0100, Ulf Hansson wrote: > > > > > > > On Wed, 29 Nov 2023 at 10:25, Avri Altman <avri.altman@wdc.com> > > wrote: > > > > > > > > > > > > > > > > Field Firmware Update (ffu) may use close-ended or open ended > > sequence. > > > > > > > > Each such sequence is comprised of a write commands enclosed > > > > > > > > between > > > > > > > > 2 switch commands - to and from ffu mode. So for the > > > > > > > > close-ended case, it will be: cmd6->cmd23-cmd25-cmd6. > > > > > > > > > > > > > > > > Some host controllers however, get confused when multi-block > > > > > > > > rw is sent without sbc, and may generate auto-cmd12 which > > > > > > > > breaks the ffu > > > > > > sequence. > > > > > > > > I encountered this issue while testing fwupd > > > > > > > > (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm > > > > > > > > based QC- > > > > > > 7c, code name - strongbad. > > > > > > > > > > > > > > > > Instead of a quirk, or hooking the request function of the > > > > > > > > msm ops, it would be better to fix the ioctl handling and > > > > > > > > make it use mrq.sbc instead of issuing SET_BLOCK_COUNT > > separately. > > > > > > > > > > > > > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > > > > > > > > > > > Applied for next (to get it tested a bit more) and by adding a > > > > > > > stable tag, thanks! > > > > > > > > > > > > This change is causing RPMB breakage in 6.6.13 and also 6.6.20. > > > > > > rockpi4b and synquacer arm64 boards with u-boot, optee 4.1.0 and > > > > > > firmware TPM (fTPM) fail to access RPMB via kernel and tee-supplicant > > 4.1.0. > > > > > > > > > > > > More details in > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > > > > > > > > > > > I've tried to identify what exactly is going wrong but failed so > > > > > > far. Reverting this changes is the only working solution so far. > > > > > > This also triggered a kernel crash on error path > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=218542 which is now > > fixed/queued. > > > > > > > > > > > > If you have any hints how to debug this further or patches to > > > > > > try, I'd be happy to try those out. > > > > > I don't know nothing about tpm nor the ftpm. > > > > > The above patch is scanning command sequences sent via > > > > > MMC_IOC_MULTI_CMD, looking for pairs of CMD23->CMD25 or > > > > > CMD23->CMD18, drops the CMD23 and build one instead in > > __mmc_blk_ioctl_cmd as the mrq.sbc. > > > > > AFAIK user-space utils, e.g. mmc-utils count on the mmc driver to > > > > > provide SBC when accessing rpmb, so their multi-ioctl sequences does not > > include CMD23, hence does not affected by this code. > > > > > > > > > > Looking in the strace included, I tried to find where > > MMC_IOC_MULTI_CMD is sent. > > > > > There are 8 such ioctl calls. > > > > > I guess the tee supplicant sources are unavailable, But it looks > > > > > like 2 simultaneous threads are trying to access the rpmb partition - which > > is forbidden. > > > > > Opening /dev/mmcblk0rpmb from user space is exclusive, so I am not > > sure how even this is possible. > > > > > > > > tee-supplicant sources are available from > > > > https://github.com/OP-TEE/optee_client > > > > and specifically > > > > https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/sr > > > > c/rpmb.c#L893 > > > > for MMC_IOC_MULTI_CMD. > I see no issue with this code. > Moreover, Since it doesn't contains any CMD23 it shouldn't interact with the above patch. > > > > > > > > > Interesting if there are two threads trying to access RPMB at the > > > > same time. Any comments here from Jens? I would have expected kernel > > > > to keep RPMB access exclusive for a single user. > > > > > > tee-supplicant is multithreaded, but OP-TEE in the secure world has a > > > global mutex to protect against concurrent access to RPMB. From the > > > secure world point of view, it's needed to manage the write counter, > > > but obviously, it has other side effects. > > > See for instance rpmb_fs_open() at > > > https://github.com/OP-TEE/optee_os/blob/master/core/tee/tee_rpmb_fs.c# > > > L2953, it's the same for all the other functions accessing RPMB. > > > > Thanks. From what I gather from strace log output in > > https://bugzilla.kernel.org/show_bug.cgi?id=218587 > > is that indeed two threads send the commands but they share the same file > > descriptor number 6 and also use mutex to make access exclusive: > > > > [pid 162] 00:00:15 openat(AT_FDCWD, "/dev/mmcblk0rpmb", O_RDWR) = 6 > > [pid 162] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000b70) = 0 > > ... > > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a084b0) = 0 > > ... > > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a085d0) = 0 > > ... > > [pid 162] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000cc0) = 0 > > ... > > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a086c0) = 0 > > ... > > [pid 159] 00:00:15 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a087e0) = 0 > > ... > > [pid 159] 00:00:16 ioctl(6, MMC_IOC_MULTI_CMD, 0xaaaac5a08900) = 0 > > ... > > [pid 162] 00:00:16 ioctl(6, MMC_IOC_MULTI_CMD, 0xffffb4000e40) = 0 > > ... > > [pid 162] 00:00:16 ioctl(3, TEE_IOC_SUPPL_RECV, {buf_len=168, > > buf_ptr={func=0, num_params=5, > > params=[{attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE|TEE_IOCTL_PARAM_ATT > > R_META, a=0, b=0, c=0}, {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, > > {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, > > {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}, > > {attr=TEE_IOCTL_PARAM_ATTR_TYPE_NONE}]}E/TC:? 0 > > E/TC:? 0 TA panicked with code 0xffff0000 > > E/LD: Status of TA bc50d971-d4c9-42c4-82cb-343fb7f37896 > > E/LD: arch: aarch64 > > E/LD: region 0: va 0x40005000 pa 0x3061b000 size 0x002000 flags rw-s (ldelf) > > E/LD: region 1: va 0x40007000 pa 0x3061d000 size 0x008000 flags r-xs (ldelf) > > E/LD: region 2: va 0x4000f000 pa 0x30625000 size 0x001000 flags rw-s (ldelf) > > E/LD: region 3: va 0x40010000 pa 0x30626000 size 0x004000 flags rw-s (ldelf) > > E/LD: region 4: va 0x40014000 pa 0x3062a000 size 0x001000 flags r--s > > E/LD: region 5: va 0x40015000 pa 0x306b2000 size 0x011000 flags rw-s (stack) > > E/LD: region 6: va 0x40026000 pa 0xe6bb4000 size 0x002000 flags rw-- > > (param) > > E/LD: region 7: va 0x4005b000 pa 0x00001000 size 0x068000 flags r-xs [0] > > E/LD: region 8: va 0x400c3000 pa 0x00069000 size 0x01f000 flags rw-s [0] > > E/LD: [0] bc50d971-d4c9-42c4-82cb-343fb7f37896 @ 0x4005b000 > > E/LD: Call stack: > > E/LD: 0x4009901c > > E/LD: 0x4005bb40 > > E/LD: 0x4005c1b8 > > E/LD: 0x4007af3c > > E/LD: 0x40093fc0 > > E/LD: 0x4005ca2c > > E/LD: 0x4009f1f4 > > E/LD: 0x40094170 > > [ 16.187823] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke > > error: 0xffff3024 > > [ 16.188548] tpm tpm0: tpm_try_transmit: send(): error -53212 > > > > I presume with strace logging tee-supplicant runs a lot slower due to logging on > > serial console too so output is a bit mixed. > > > > Without strace the fTPM trusted application crash seems to happen > > immediately after "modprobe tpm_ftpm_tee". The TA side crash traces point to > > problems accessing data in secure storage, in RPMB via optee, kernel optee > > driver and userspace tee-supplicant. > I noticed that the code is accessing RPMB as part of fat read? Is this something that can explain the above contention? Both reads and writes are protected with the same mutex. I believe that it's just one operation followed immediately by a second one. OP-TEE doesn't cache much so we often have to read before writing. > > Anyway, it might be helpful turning on the mmc dynamic debug - > However Vanishingly unlikely, since the rpmb and the mmc device has 2 separate device nodes, > Maybe there are 2 concurrent ioctls? It may be worth trying with a single-threaded tee-supplicant to be able to rule it out. > AFAIK it doesn't get synchronized in the driver level. OK. Thanks, Jens > > Thanks, > Avri > > > > Cheers, > > > > -Mikko > > > > > Cheers, > > > Jens > > > > > > > > > > > Cheers, > > > > > > > > -Mikko > > > > > > > > > I can try and help you debug this - you can contact me offline. > > > > > > > > > > Thanks, > > > > > Avri > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > -Mikko > > > > > > > > > > > > > Kind regards > > > > > > > Uffe > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > Changelog: > > > > > > > > v3--v4: > > > > > > > > check sbc.error as well > > > > > > > > v2--v3: > > > > > > > > Adopt Adrian's proposal > > > > > > > > v1--v2: > > > > > > > > remove redundant reference of reliable write > > > > > > > > --- > > > > > > > > drivers/mmc/core/block.c | 46 > > > > > > > > +++++++++++++++++++++++++++++++++++++--- > > > > > > > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/mmc/core/block.c > > > > > > > > b/drivers/mmc/core/block.c index f9a5cffa64b1..892e74e611a0 > > > > > > > > 100644 > > > > > > > > --- a/drivers/mmc/core/block.c > > > > > > > > +++ b/drivers/mmc/core/block.c > > > > > > > > @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { > > > > > > > > struct mmc_ioc_cmd ic; > > > > > > > > unsigned char *buf; > > > > > > > > u64 buf_bytes; > > > > > > > > + unsigned int flags; > > > > > > > > +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ > > > > > > > > +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ > > > > > > > > + > > > > > > > > struct mmc_rpmb_data *rpmb; }; > > > > > > > > > > > > > > > > @@ -465,7 +469,7 @@ static int > > > > > > > > mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user > > > > > > > > *ic_ptr, } > > > > > > > > > > > > > > > > static int __mmc_blk_ioctl_cmd(struct mmc_card *card, > > > > > > > > struct > > > > > > mmc_blk_data *md, > > > > > > > > - struct mmc_blk_ioc_data *idata) > > > > > > > > + struct mmc_blk_ioc_data > > > > > > > > + **idatas, int > > > > > > > > + i) > > > > > > > > { > > > > > > > > struct mmc_command cmd = {}, sbc = {}; > > > > > > > > struct mmc_data data = {}; @@ -475,10 +479,18 @@ > > > > > > > > static int __mmc_blk_ioctl_cmd(struct > > > > > > mmc_card *card, struct mmc_blk_data *md, > > > > > > > > unsigned int busy_timeout_ms; > > > > > > > > int err; > > > > > > > > unsigned int target_part; > > > > > > > > + struct mmc_blk_ioc_data *idata = idatas[i]; > > > > > > > > + struct mmc_blk_ioc_data *prev_idata = NULL; > > > > > > > > > > > > > > > > if (!card || !md || !idata) > > > > > > > > return -EINVAL; > > > > > > > > > > > > > > > > + if (idata->flags & MMC_BLK_IOC_DROP) > > > > > > > > + return 0; > > > > > > > > + > > > > > > > > + if (idata->flags & MMC_BLK_IOC_SBC) > > > > > > > > + prev_idata = idatas[i - 1]; > > > > > > > > + > > > > > > > > /* > > > > > > > > * The RPMB accesses comes in from the character device, so > > we > > > > > > > > * need to target these explicitly. Else we just > > > > > > > > target the @@ -532,7 +544,7 @@ static int > > > > > > > > __mmc_blk_ioctl_cmd(struct mmc_card > > > > > > *card, struct mmc_blk_data *md, > > > > > > > > return err; > > > > > > > > } > > > > > > > > > > > > > > > > - if (idata->rpmb) { > > > > > > > > + if (idata->rpmb || prev_idata) { > > > > > > > > sbc.opcode = MMC_SET_BLOCK_COUNT; > > > > > > > > /* > > > > > > > > * We don't do any blockcount validation > > > > > > > > because the max size @@ -540,6 +552,8 @@ static int > > > > > > > > __mmc_blk_ioctl_cmd(struct > > > > > > mmc_card *card, struct mmc_blk_data *md, > > > > > > > > * 'Reliable Write' bit here. > > > > > > > > */ > > > > > > > > sbc.arg = data.blocks | > > > > > > > > (idata->ic.write_flag & BIT(31)); > > > > > > > > + if (prev_idata) > > > > > > > > + sbc.arg = prev_idata->ic.arg; > > > > > > > > sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > > > > > > > > mrq.sbc = &sbc; > > > > > > > > } > > > > > > > > @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct > > > > > > > > mmc_card > > > > > > *card, struct mmc_blk_data *md, > > > > > > > > mmc_wait_for_req(card->host, &mrq); > > > > > > > > memcpy(&idata->ic.response, cmd.resp, > > > > > > > > sizeof(cmd.resp)); > > > > > > > > > > > > > > > > + if (prev_idata) { > > > > > > > > + memcpy(&prev_idata->ic.response, sbc.resp, > > sizeof(sbc.resp)); > > > > > > > > + if (sbc.error) { > > > > > > > > + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", > > > > > > > > + __func__, sbc.error); > > > > > > > > + return sbc.error; > > > > > > > > + } > > > > > > > > + } > > > > > > > > + > > > > > > > > if (cmd.error) { > > > > > > > > dev_err(mmc_dev(card->host), "%s: cmd error %d\n", > > > > > > > > __func__, > > > > > > > > cmd.error); @@ -1032,6 +1055,20 @@ static inline void > > > > > > mmc_blk_reset_success(struct mmc_blk_data *md, int type) > > > > > > > > md->reset_done &= ~type; } > > > > > > > > > > > > > > > > +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) { > > > > > > > > + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; > > > > > > > > + int i; > > > > > > > > + > > > > > > > > + for (i = 1; i < mq_rq->ioc_count; i++) { > > > > > > > > + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT > > && > > > > > > > > + mmc_op_multi(idata[i]->ic.opcode)) { > > > > > > > > + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; > > > > > > > > + idata[i]->flags |= MMC_BLK_IOC_SBC; > > > > > > > > + } > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > /* > > > > > > > > * The non-block commands come back from the block layer > > > > > > > > after it queued > > > > > > it and > > > > > > > > * processed it with all other requests and then they get > > > > > > > > issued in this @@ -1059,11 +1096,14 @@ static void > > > > > > > > mmc_blk_issue_drv_op(struct > > > > > > mmc_queue *mq, struct request *req) > > > > > > > > if (ret) > > > > > > > > break; > > > > > > > > } > > > > > > > > + > > > > > > > > + mmc_blk_check_sbc(mq_rq); > > > > > > > > + > > > > > > > > fallthrough; > > > > > > > > case MMC_DRV_OP_IOCTL_RPMB: > > > > > > > > idata = mq_rq->drv_op_data; > > > > > > > > for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { > > > > > > > > - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); > > > > > > > > + ret = __mmc_blk_ioctl_cmd(card, md, > > > > > > > > + idata, i); > > > > > > > > if (ret) > > > > > > > > break; > > > > > > > > } > > > > > > > > -- > > > > > > > > 2.42.0 > > > > > > > >
Hi,
With help from Jens we turned tee-opplicant in userspace to single threaded with:
--- a/tee-supplicant/src/tee_supplicant.c
+++ b/tee-supplicant/src/tee_supplicant.c
@@ -588,6 +588,8 @@ static bool spawn_thread(struct thread_arg *arg)
int e = 0;
pthread_t tid;
+ return true;
+
memset(&tid, 0, sizeof(tid));
DMSG("Spawning a new thread");
but RPMB access still fails, so issue is not in userspace concurrency.
I added debug prints to this commit and the failures seem to come from
this first check idata->flags & MMC_BLK_IOC_DROP, second hunk in this patch:
@@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
unsigned int busy_timeout_ms;
int err;
unsigned int target_part;
+ struct mmc_blk_ioc_data *idata = idatas[i];
+ struct mmc_blk_ioc_data *prev_idata = NULL;
if (!card || !md || !idata)
return -EINVAL;
+ if (idata->flags & MMC_BLK_IOC_DROP)
+ return 0;
+
+ if (idata->flags & MMC_BLK_IOC_SBC)
+ prev_idata = idatas[i - 1];
+
/*
* The RPMB accesses comes in from the character device, so we
* need to target these explicitly. Else we just target the
Debug prints:
@@ -485,11 +485,19 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
if (!card || !md || !idata)
return -EINVAL;
- if (idata->flags & MMC_BLK_IOC_DROP)
+ pr_err("%s: DEBUG0:\n", __func__ );
+
+ if (idata->flags & MMC_BLK_IOC_DROP) {
+ pr_err("%s: DEBUG1: idata->flags & MMC_BLK_IOC_DROP: flags = 0x%x, returning 0\n",
+ __func__, idata->flags );
return 0;
+ }
- if (idata->flags & MMC_BLK_IOC_SBC)
+ if (idata->flags & MMC_BLK_IOC_SBC && i > 0) {
+ pr_err("%s: DEBUG2: idata->flags & MMC_BLK_IOC_SBC && i > 0, flags = 0x%x\n",
+ __func__, idata->flags);
prev_idata = idatas[i - 1];
+ }
/*
* The RPMB accesses comes in from the character device, so we
And the logs show that "idata->flags & MMC_BLK_IOC_DROP" is always true for the RPMB
ioctls.
https://ledge.validation.linaro.org/scheduler/job/83101
[ 33.505035] __mmc_blk_ioctl_cmd: DEBUG0:
[ 33.505426] __mmc_blk_ioctl_cmd: DEBUG1: idata->flags & MMC_BLK_IOC_DROP: flags = 0x5f797469, returning 0
[ 33.506283] __mmc_blk_ioctl_cmd: DEBUG0:
[ 33.506639] __mmc_blk_ioctl_cmd: DEBUG2: idata->flags & MMC_BLK_IOC_SBC && i > 0, flags = 0x702e796e
[ 33.507447] __mmc_blk_ioctl_cmd: DEBUG2.1: RPMB
[ 33.511746] __mmc_blk_ioctl_cmd: DEBUG3: copying to prev_idata
[ 43.564084] mmc0: Card stuck being busy! __mmc_poll_for_busy
https://ledge.validation.linaro.org/scheduler/job/83102
[ 143.124673] __mmc_blk_ioctl_cmd: DEBUG2: idata->flags & MMC_BLK_IOC_SBC && i > 0, flags = 0x485fb78a
[ 143.133854] __mmc_blk_ioctl_cmd: DEBUG2.1: RPMB
[ 143.138886] __mmc_blk_ioctl_cmd: DEBUG3: copying to prev_idata
...
[ 153.166684] mmc0: Card stuck being busy! __mmc_poll_for_busy
This commit added uint flags to mmc_blk_ioc_data struct but it is only initialized for
MMC_DRV_OP_IOCTL code path and for MMC_DRV_OP_IOCTL_RPMB it is uninialized and happens to
be matching "& MMC_BLK_IOC_DROP" in all cases at runtime thus breaking RPMB ioctls.
Fix will be to initialize mmc_blk_ioc_data->flags in all cases. Would this be fine as
a catch all initialization for mmc_blk_ioc_data?
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -413,7 +413,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
struct mmc_blk_ioc_data *idata;
int err;
- idata = kmalloc(sizeof(*idata), GFP_KERNEL);
+ idata = kzalloc(sizeof(*idata), GFP_KERNEL);
if (!idata) {
err = -ENOMEM;
goto out;
I think this is a sensible and future proof way to go.
Then, the second flags check for MMC_BLK_IOC_SBC is accessing array using
index i - 1, but is not checking if i == 0 which results in data[-1] access.
I think this should be fixed with something like:
- if (idata->flags & MMC_BLK_IOC_SBC)
+ if (idata->flags & MMC_BLK_IOC_SBC && i > 0) {
Would you be fine with this?
With both of these changes in place (and debug logging) test runs on rockpi4b and synquacer
arm64 boards are now ok and firmware TPM devices with RPMB storage works again and optee fTPM TA does
not panic, though there is at least on TPM eventlog read test failing later on (a different kernel or
firmware bug, perhaps).
https://ledge.validation.linaro.org/scheduler/job/83094
+ tee-supplicant -d --rpmb-cid 7001004d33323530385212b201dea300
+ sleep 10
+ modprobe tpm_ftpm_tee
...
+ tpm2_createprimary -Q --hierarchy=o --key-context=prim.ctx
...
+ tpm2_loadexternal --key-algorithm=rsa --hierarchy=o --public=signing_key_public.pem --key-context=signing_key.ctx --name=signing_key.name
+ tpm2_startauthsession --session=session.ctx
+ tpm2_policyauthorize --session=session.ctx --policy=authorized.policy --name=signing_key.name
+ tpm2_flushcontext session.ctx
+ cat /tmp/rand_key
+ tpm2_create --hash-algorithm=sha256 --public=auth_pcr_seal_key.pub --private=auth_pcr_seal_key.priv --sealing-input=- --parent-context=prim.ctx --policy=authorized.policy
...
+ tpm2_load -Q --parent-context=prim.ctx --public=auth_pcr_seal_key.pub --private=auth_pcr_seal_key.priv --name=seal.name --key-context=seal.ctx
+ tpm2_evictcontrol -Q -C o -c 0x8100000a
...
+ cryptsetup -q --type luks2 --cipher aes-xts-plain --hash sha256 --use-random --uuid=6091b3a4-ce08-3020-93a6-f755a22ef03b luksFormat /dev/sda2 --key-file /tmp/rand_key --label otaroot
+ echo 'Creating encrypted filesystem ...'
Creating encrypted filesystem ...
+ cryptsetup luksOpen --key-file /tmp/rand_key /dev/sda2 rootfs
...
https://ledge.validation.linaro.org/scheduler/job/83096
+ modprobe tpm_ftpm_tee
...
+ rngd
...
+ tpm2_dictionarylockout -c
+ tpm2-abrmd --allow-root
...
+ tpm2_seal_password /tmp/rand_key
+ local passfilename=/tmp/rand_key
...
+ tpm2_createprimary -Q --hierarchy=o --key-context=prim.ctx
...
+ tpm2_loadexternal --key-algorithm=rsa --hierarchy=o --public=signing_key_public.pem --key-context=signing_key.ctx --name=signing_key.name
+ tpm2_startauthsession --session=session.ctx
+ tpm2_policyauthorize --session=session.ctx --policy=authorized.policy --name=signing_key.name
+ tpm2_flushcontext session.ctx
...
+ tpm2_create --hash-algorithm=sha256 --public=auth_pcr_seal_key.pub --private=auth_pcr_seal_key.priv --sealing-input=- --parent-context=prim.ctx --policy=authorized.policy
...
+ tpm2_load -Q --parent-context=prim.ctx --public=auth_pcr_seal_key.pub --private=auth_pcr_seal_key.priv --name=seal.name --key-context=seal.ctx
+ tpm2_evictcontrol -Q -C o -c 0x8100000a
+ tpm2_evictcontrol --hierarchy=o --object-context=seal.ctx 0x8100000a
...
+ cryptsetup -q --type luks2 --cipher aes-xts-plain --hash sha256 --use-random --uuid=6091b3a4-ce08-3020-93a6-f755a22ef03b luksFormat /dev/mmcblk1p7 --key-file /tmp/rand_key --label otaroot
...
Creating encrypted filesystem ...
+ cryptsetup luksOpen --key-file /tmp/rand_key /dev/mmcblk1p7 rootfs
...
+ mount /dev/mapper/rootfs /rootfs -o rw,noatime,iversion
[ 171.018740] EXT4-fs (dm-0): mounted filesystem 89ae0ee0-b27c-4a66-ac0c-098c7ccd7a3c r/w with ordered data mode. Quota mode: disabled.
...
Cheers,
-Mikko
On Wed, Mar 13, 2024 at 01:46:44PM +0200, Mikko Rapeli wrote: <snip> > Fix will be to initialize mmc_blk_ioc_data->flags in all cases. Would this be fine as > a catch all initialization for mmc_blk_ioc_data? > > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -413,7 +413,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( > struct mmc_blk_ioc_data *idata; > int err; > > - idata = kmalloc(sizeof(*idata), GFP_KERNEL); > + idata = kzalloc(sizeof(*idata), GFP_KERNEL); > if (!idata) { > err = -ENOMEM; > goto out; > > I think this is a sensible and future proof way to go. > > Then, the second flags check for MMC_BLK_IOC_SBC is accessing array using > index i - 1, but is not checking if i == 0 which results in data[-1] access. > I think this should be fixed with something like: > > - if (idata->flags & MMC_BLK_IOC_SBC) > + if (idata->flags & MMC_BLK_IOC_SBC && i > 0) { > > Would you be fine with this? In addition to these, is the original patch correct also when idata->rpmb is not true but prev_idata is? I have no idea of the code but this looks a bit suspicious. @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, return err; } - if (idata->rpmb) { + if (idata->rpmb || prev_idata) { sbc.opcode = MMC_SET_BLOCK_COUNT; /* * We don't do any blockcount validation because the max size @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, * 'Reliable Write' bit here. */ sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31)); + if (prev_idata) + sbc.arg = prev_idata->ic.arg; sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; mrq.sbc = &sbc; } Cheers, -Mikko
+ Tomas > > Hi, > > With help from Jens we turned tee-opplicant in userspace to single threaded > with: > > --- a/tee-supplicant/src/tee_supplicant.c > +++ b/tee-supplicant/src/tee_supplicant.c > @@ -588,6 +588,8 @@ static bool spawn_thread(struct thread_arg *arg) > int e = 0; > pthread_t tid; > > + return true; > + > memset(&tid, 0, sizeof(tid)); > > DMSG("Spawning a new thread"); > > > but RPMB access still fails, so issue is not in userspace concurrency. > I added debug prints to this commit and the failures seem to come from this > first check idata->flags & MMC_BLK_IOC_DROP, second hunk in this patch: > > @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > *card, struct mmc_blk_data *md, > unsigned int busy_timeout_ms; > int err; > unsigned int target_part; > + struct mmc_blk_ioc_data *idata = idatas[i]; > + struct mmc_blk_ioc_data *prev_idata = NULL; > > if (!card || !md || !idata) > return -EINVAL; > > + if (idata->flags & MMC_BLK_IOC_DROP) > + return 0; > + > + if (idata->flags & MMC_BLK_IOC_SBC) > + prev_idata = idatas[i - 1]; > + > /* > * The RPMB accesses comes in from the character device, so we > * need to target these explicitly. Else we just target the > > > Debug prints: > > @@ -485,11 +485,19 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > *card, struct mmc_blk_data *md, > if (!card || !md || !idata) > return -EINVAL; > > - if (idata->flags & MMC_BLK_IOC_DROP) > + pr_err("%s: DEBUG0:\n", __func__ ); > + > + if (idata->flags & MMC_BLK_IOC_DROP) { > + pr_err("%s: DEBUG1: idata->flags & MMC_BLK_IOC_DROP: flags = > 0x%x, returning 0\n", > + __func__, > + idata->flags ); > return 0; > + } > > - if (idata->flags & MMC_BLK_IOC_SBC) > + if (idata->flags & MMC_BLK_IOC_SBC && i > 0) { > + pr_err("%s: DEBUG2: idata->flags & MMC_BLK_IOC_SBC && i > 0, > flags = 0x%x\n", > + __func__, > + idata->flags); > prev_idata = idatas[i - 1]; > + } > > /* > * The RPMB accesses comes in from the character device, so we > > And the logs show that "idata->flags & MMC_BLK_IOC_DROP" is always true > for the RPMB ioctls. > > https://ledge.validation.linaro.org/scheduler/job/83101 > > [ 33.505035] __mmc_blk_ioctl_cmd: DEBUG0: > [ 33.505426] __mmc_blk_ioctl_cmd: DEBUG1: idata->flags & > MMC_BLK_IOC_DROP: flags = 0x5f797469, returning 0 The flags contains garbage - needs to be initialized to zero. Tomas already noticed that and is about to send a fix. Thanks, Avri > [ 33.506283] __mmc_blk_ioctl_cmd: DEBUG0: > [ 33.506639] __mmc_blk_ioctl_cmd: DEBUG2: idata->flags & > MMC_BLK_IOC_SBC && i > 0, flags = 0x702e796e > [ 33.507447] __mmc_blk_ioctl_cmd: DEBUG2.1: RPMB > [ 33.511746] __mmc_blk_ioctl_cmd: DEBUG3: copying to prev_idata > [ 43.564084] mmc0: Card stuck being busy! __mmc_poll_for_busy > > https://ledge.validation.linaro.org/scheduler/job/83102 > > [ 143.124673] __mmc_blk_ioctl_cmd: DEBUG2: idata->flags & > MMC_BLK_IOC_SBC && i > 0, flags = 0x485fb78a [ 143.133854] > __mmc_blk_ioctl_cmd: DEBUG2.1: RPMB [ 143.138886] > __mmc_blk_ioctl_cmd: DEBUG3: copying to prev_idata ... > [ 153.166684] mmc0: Card stuck being busy! __mmc_poll_for_busy > > This commit added uint flags to mmc_blk_ioc_data struct but it is only > initialized for MMC_DRV_OP_IOCTL code path and for > MMC_DRV_OP_IOCTL_RPMB it is uninialized and happens to be matching "& > MMC_BLK_IOC_DROP" in all cases at runtime thus breaking RPMB ioctls. > > Fix will be to initialize mmc_blk_ioc_data->flags in all cases. Would this be fine > as a catch all initialization for mmc_blk_ioc_data? > > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -413,7 +413,7 @@ static struct mmc_blk_ioc_data > *mmc_blk_ioctl_copy_from_user( > struct mmc_blk_ioc_data *idata; > int err; > > - idata = kmalloc(sizeof(*idata), GFP_KERNEL); > + idata = kzalloc(sizeof(*idata), GFP_KERNEL); > if (!idata) { > err = -ENOMEM; > goto out; > > I think this is a sensible and future proof way to go. > > Then, the second flags check for MMC_BLK_IOC_SBC is accessing array using > index i - 1, but is not checking if i == 0 which results in data[-1] access. > I think this should be fixed with something like: > > - if (idata->flags & MMC_BLK_IOC_SBC) > + if (idata->flags & MMC_BLK_IOC_SBC && i > 0) { > > Would you be fine with this? > > With both of these changes in place (and debug logging) test runs on rockpi4b > and synquacer > arm64 boards are now ok and firmware TPM devices with RPMB storage works > again and optee fTPM TA does not panic, though there is at least on TPM > eventlog read test failing later on (a different kernel or firmware bug, perhaps). > > https://ledge.validation.linaro.org/scheduler/job/83094 > > + tee-supplicant -d --rpmb-cid 7001004d33323530385212b201dea300 sleep 10 > + modprobe tpm_ftpm_tee > ... > + tpm2_createprimary -Q --hierarchy=o --key-context=prim.ctx > ... > + tpm2_loadexternal --key-algorithm=rsa --hierarchy=o > + --public=signing_key_public.pem --key-context=signing_key.ctx > + --name=signing_key.name tpm2_startauthsession --session=session.ctx > + tpm2_policyauthorize --session=session.ctx --policy=authorized.policy > + --name=signing_key.name tpm2_flushcontext session.ctx cat > + /tmp/rand_key tpm2_create --hash-algorithm=sha256 > + --public=auth_pcr_seal_key.pub --private=auth_pcr_seal_key.priv > + --sealing-input=- --parent-context=prim.ctx --policy=authorized.policy > ... > + tpm2_load -Q --parent-context=prim.ctx --public=auth_pcr_seal_key.pub > + --private=auth_pcr_seal_key.priv --name=seal.name > + --key-context=seal.ctx tpm2_evictcontrol -Q -C o -c 0x8100000a > ... > + cryptsetup -q --type luks2 --cipher aes-xts-plain --hash sha256 > + --use-random --uuid=6091b3a4-ce08-3020-93a6-f755a22ef03b luksFormat > /dev/sda2 --key-file /tmp/rand_key --label otaroot echo 'Creating encrypted > filesystem ...' > Creating encrypted filesystem ... > + cryptsetup luksOpen --key-file /tmp/rand_key /dev/sda2 rootfs > ... > > https://ledge.validation.linaro.org/scheduler/job/83096 > > + modprobe tpm_ftpm_tee > ... > + rngd > ... > + tpm2_dictionarylockout -c > + tpm2-abrmd --allow-root > ... > + tpm2_seal_password /tmp/rand_key > + local passfilename=/tmp/rand_key > ... > + tpm2_createprimary -Q --hierarchy=o --key-context=prim.ctx > ... > + tpm2_loadexternal --key-algorithm=rsa --hierarchy=o > + --public=signing_key_public.pem --key-context=signing_key.ctx > + --name=signing_key.name tpm2_startauthsession --session=session.ctx > + tpm2_policyauthorize --session=session.ctx --policy=authorized.policy > + --name=signing_key.name tpm2_flushcontext session.ctx > ... > + tpm2_create --hash-algorithm=sha256 --public=auth_pcr_seal_key.pub > + --private=auth_pcr_seal_key.priv --sealing-input=- > + --parent-context=prim.ctx --policy=authorized.policy > ... > + tpm2_load -Q --parent-context=prim.ctx --public=auth_pcr_seal_key.pub > + --private=auth_pcr_seal_key.priv --name=seal.name > + --key-context=seal.ctx tpm2_evictcontrol -Q -C o -c 0x8100000a > + tpm2_evictcontrol --hierarchy=o --object-context=seal.ctx 0x8100000a > ... > + cryptsetup -q --type luks2 --cipher aes-xts-plain --hash sha256 > + --use-random --uuid=6091b3a4-ce08-3020-93a6-f755a22ef03b luksFormat > + /dev/mmcblk1p7 --key-file /tmp/rand_key --label otaroot > ... > Creating encrypted filesystem ... > + cryptsetup luksOpen --key-file /tmp/rand_key /dev/mmcblk1p7 rootfs > ... > + mount /dev/mapper/rootfs /rootfs -o rw,noatime,iversion > [ 171.018740] EXT4-fs (dm-0): mounted filesystem 89ae0ee0-b27c-4a66-ac0c- > 098c7ccd7a3c r/w with ordered data mode. Quota mode: disabled. > ... > > Cheers, > > -Mikko
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index f9a5cffa64b1..892e74e611a0 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -400,6 +400,10 @@ struct mmc_blk_ioc_data { struct mmc_ioc_cmd ic; unsigned char *buf; u64 buf_bytes; + unsigned int flags; +#define MMC_BLK_IOC_DROP BIT(0) /* drop this mrq */ +#define MMC_BLK_IOC_SBC BIT(1) /* use mrq.sbc */ + struct mmc_rpmb_data *rpmb; }; @@ -465,7 +469,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, } static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, - struct mmc_blk_ioc_data *idata) + struct mmc_blk_ioc_data **idatas, int i) { struct mmc_command cmd = {}, sbc = {}; struct mmc_data data = {}; @@ -475,10 +479,18 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, unsigned int busy_timeout_ms; int err; unsigned int target_part; + struct mmc_blk_ioc_data *idata = idatas[i]; + struct mmc_blk_ioc_data *prev_idata = NULL; if (!card || !md || !idata) return -EINVAL; + if (idata->flags & MMC_BLK_IOC_DROP) + return 0; + + if (idata->flags & MMC_BLK_IOC_SBC) + prev_idata = idatas[i - 1]; + /* * The RPMB accesses comes in from the character device, so we * need to target these explicitly. Else we just target the @@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, return err; } - if (idata->rpmb) { + if (idata->rpmb || prev_idata) { sbc.opcode = MMC_SET_BLOCK_COUNT; /* * We don't do any blockcount validation because the max size @@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, * 'Reliable Write' bit here. */ sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31)); + if (prev_idata) + sbc.arg = prev_idata->ic.arg; sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; mrq.sbc = &sbc; } @@ -557,6 +571,15 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, mmc_wait_for_req(card->host, &mrq); memcpy(&idata->ic.response, cmd.resp, sizeof(cmd.resp)); + if (prev_idata) { + memcpy(&prev_idata->ic.response, sbc.resp, sizeof(sbc.resp)); + if (sbc.error) { + dev_err(mmc_dev(card->host), "%s: sbc error %d\n", + __func__, sbc.error); + return sbc.error; + } + } + if (cmd.error) { dev_err(mmc_dev(card->host), "%s: cmd error %d\n", __func__, cmd.error); @@ -1032,6 +1055,20 @@ static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type) md->reset_done &= ~type; } +static void mmc_blk_check_sbc(struct mmc_queue_req *mq_rq) +{ + struct mmc_blk_ioc_data **idata = mq_rq->drv_op_data; + int i; + + for (i = 1; i < mq_rq->ioc_count; i++) { + if (idata[i - 1]->ic.opcode == MMC_SET_BLOCK_COUNT && + mmc_op_multi(idata[i]->ic.opcode)) { + idata[i - 1]->flags |= MMC_BLK_IOC_DROP; + idata[i]->flags |= MMC_BLK_IOC_SBC; + } + } +} + /* * The non-block commands come back from the block layer after it queued it and * processed it with all other requests and then they get issued in this @@ -1059,11 +1096,14 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req) if (ret) break; } + + mmc_blk_check_sbc(mq_rq); + fallthrough; case MMC_DRV_OP_IOCTL_RPMB: idata = mq_rq->drv_op_data; for (i = 0, ret = 0; i < mq_rq->ioc_count; i++) { - ret = __mmc_blk_ioctl_cmd(card, md, idata[i]); + ret = __mmc_blk_ioctl_cmd(card, md, idata, i); if (ret) break; }
Field Firmware Update (ffu) may use close-ended or open ended sequence. Each such sequence is comprised of a write commands enclosed between 2 switch commands - to and from ffu mode. So for the close-ended case, it will be: cmd6->cmd23-cmd25-cmd6. Some host controllers however, get confused when multi-block rw is sent without sbc, and may generate auto-cmd12 which breaks the ffu sequence. I encountered this issue while testing fwupd (github.com/fwupd/fwupd) on HP Chromebook x2, a qualcomm based QC-7c, code name - strongbad. Instead of a quirk, or hooking the request function of the msm ops, it would be better to fix the ioctl handling and make it use mrq.sbc instead of issuing SET_BLOCK_COUNT separately. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- Changelog: v3--v4: check sbc.error as well v2--v3: Adopt Adrian's proposal v1--v2: remove redundant reference of reliable write --- drivers/mmc/core/block.c | 46 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-)