On Mon, Mar 11, 2024 at 8:25 PM Avri Altman <Avri.Altman@xxxxxxx> 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@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/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 > > > > > > > >
