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@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. 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 > > > > > >
