On Mon, Mar 11, 2024 at 4:08 PM Mikko Rapeli <mikko.rapeli@xxxxxxxxxx> 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@xxxxxxx> 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@xxxxxxx> > > > > > > > > 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 > > > > >
