RE: [PATCH v4] mmc: core: Use mrq.sbc in close-ended ffu

[Avri Altman <Avri.Altman@wdc.com>]

+ 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