Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

[Jan Kiszka]

On 07.06.23 17:18, Sumit Garg wrote:
> On Wed, 7 Jun 2023 at 18:10, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>
>> On 07.06.23 10:25, Ilias Apalodimas wrote:
>>> Hi Sumit,
>>>
>>> On Wed, 7 Jun 2023 at 10:25, Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
>>>>
>>>> Hi Ilias,
>>>>
>>>> On Wed, 7 Jun 2023 at 12:05, Ilias Apalodimas
>>>> <ilias.apalodimas@xxxxxxxxxx> wrote:
>>>>>
>>>>> Hi Jan,
>>>>>
>>>>> [...]
>>>>>
>>>>>>>>>>
>>>>>>> ...
>>>>>>>>>
>>>>>>>>> I think we have a probe ordering issue with this driver:
>>>>>>>>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
>>>>>>>>> with the default efivar ops still registered. And that means
>>>>>>>>> efivar_supports_writes() will return false, and the fs declares itself
>>>>>>>>> as readonly. I've seen systemd mounting it r/o initialling, and you need
>>>>>>>>> to remount the fs to enable writability.
>>>>>>>>>
>>>>>>>>> Is there anything that could be done to re-order things reliably, probe
>>>>>>>>> the tee bus earlier etc.?
>>>>>>>>
>>>>>>>> This driver has a dependency on user-space daemon: tee-supplicant to
>>>>>>>> be running for RPMB access. So once you start that daemon the
>>>>>>>> corresponding device will be enumerated on the TEE bus and this driver
>>>>>>>> probe will be invoked. So I would suggest you to load this daemon very
>>>>>>>> early in the boot process or better to make it a part of initramfs.
>>>>>>>>
>>>>>>>
>>>>>>> That is not the point, really.
>>>>>>>
>>>>>>> If this dependency exists, the code should be aware of that, and made
>>>>>>> to work correctly in spite of it. Requiring a module to be part of
>>>>>>> initramfs is not a reasonable fix.
>>>>>>
>>>>>> In fact, I've tested a non-modularized build as well, just to exclude
>>>>>> that issue. The daemon dependency is more likely the problem here.
>>>>>>
>>>>>>>
>>>>>>> IIUC, this also means that the efivar ops are updated while there is
>>>>>>> already a client. This seems less than ideal as well
>>>>>
>>>>> As Sumit pointed out, the 'device' won't be available from OP-TEE
>>>>> until the supplicant is up and running and as a result, the module
>>>>> _probe() function won't run.  Unfortunately, this isn't something we
>>>>> can avoid since the supplicant is responsible for the RPMB writes.
>>>>> The only thing I can think of is moving parts of the supplicant to the
>>>>> kernel and wiring up the RPC calls for reading/writing data to the
>>>>> eMMC subsystem.  There was another discussion here [0] requesting the
>>>>> same thing for different reasons. But unless I am missing something
>>>>> this won't solve the problem completely either.  You still have a
>>>>> timing dependency of "when did the RT callbacks change" -- "when was
>>>>> my efivarfs mounted".
>>>>
>>>> With the RPMB writes wired through the kernel [1], the only dependency
>>>> left is when do you load the tee-stmm-efi driver to have real EFI
>>>> runtime variables support. IMO, tee-stmm-efi driver should be built-in
>>>> to support systems without initramfs. The distro installers may choose
>>>> to bundle it in initramfs. Do you still see a timing dependency with
>>>> this approach?
>>>
>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>> As you point out you will still have this dependency if you end up
>>> building them as modules and you manage to mount the efivarfs before
>>> those get inserted.  Does anyone see a reasonable workaround?
>>> Deceiving the kernel and making the bootloader set the RT property bit
>>> to force the filesystem being mounted as rw is a nasty hack that we
>>> should avoid.  Maybe adding a kernel command line parameter that says
>>> "Ignore the RTPROP I know what I am doing"?  I don't particularly love
>>> this either, but it's not unreasonable.
>>
>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>> basically this issue mapped on reboot/shutdown, I would really love to
>> see the unhandy tee-supplicant daemon to be overcome.
> 
> I have seen this error before and it has been on my todo list. So I
> have tried to fix it here [1]. Feel free to test it and let me know if
> you see any further issues.
> 
> [1] https://lkml.org/lkml/2023/6/7/927
> 

Ah, nice, will test ASAP!

Meanwhile more food: I managed to build a firmware that was missing 
STMM. But the driver loaded, and I got this:

root@iot2050-debian:~# efi-updatevar -f PK.auth PK
[  243.407097] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[  243.415959] Mem abort info:
[  243.418801]   ESR = 0x86000004
[  243.422099]   EC = 0x21: IABT (current EL), IL = 32 bits
[  243.427529]   SET = 0, FnV = 0
[  243.430755]   EA = 0, S1PTW = 0
[  243.433931] user pgtable: 4k pages, 48-bit VAs, pgdp=000000008b74e000
[  243.440438] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[  243.447274] Internal error: Oops: 86000004 [#1] PREEMPT SMP
[  243.452835] Modules linked in: ctr ccm mt7601u mac80211 cfg80211 rfkill libarc4 cp210x usbserial pci_endpoint_test ti_k3_r5_remoteproc optee_rng rng_core ti_cal ti_am335x_adc videobuf2_dma_contig kfifo_buf v4l2_fwnode videobuf2_memops videobuf2_v4l2 videobuf2_common irq_pruss_intc at24 fuse ip_tables x_tables ipv6 tpm_ftpm_tee icssg_prueth pru_rproc icss_iep ptp pps_core ti_am335x_tscadc pruss
[  243.487733] CPU: 0 PID: 875 Comm: efi-updatevar Not tainted 5.10.162-cip24 #1
[  243.494851] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2023.07-rc3-00018-g0afdaac6505 07/01/2023
[  243.505180] pstate: 00000005 (nzcv daif -PAN -UAO -TCO BTYPE=--)
[  243.511179] pc : 0x0
[  243.513366] lr : efivar_entry_set_get_size+0xd4/0x1e0
[  243.518404] sp : ffff8000127a3d00
[  243.521708] x29: ffff8000127a3d00 x28: 0000000000000f87 
[  243.527012] x27: ffff000005bae400 x26: ffff800011254000 
[  243.532315] x25: ffff000005baf000 x24: ffff800011254aa0 
[  243.537618] x23: ffff8000127a3dab x22: ffff8000111d0268 
[  243.542921] x21: ffff8000127a3db0 x20: 0000000000000000 
[  243.548224] x19: ffff000005bae000 x18: 0000000000000000 
[  243.553527] x17: 0000000000000000 x16: 0000000000000000 
[  243.558829] x15: 0000aaab0876f264 x14: 35cd6a1025d11a20 
[  243.564132] x13: ac6f8dda3945638d x12: fb482642e3487f2d 
[  243.569435] x11: 0000000000000003 x10: ffff00000b792a80 
[  243.574738] x9 : a098e2bf989ff097 x8 : 0000000000000010 
[  243.580041] x7 : ffff800010e35c00 x6 : 4ddcbe2ecfc8fc79 
[  243.585345] x5 : 0000000000000000 x4 : ffff000005baf000 
[  243.590647] x3 : 0000000000000f87 x2 : 0000000000000027 
[  243.595950] x1 : ffff000005bae400 x0 : ffff000005bae000 
[  243.601254] Call trace:
[  243.603695]  0x0
[  243.605531]  efivarfs_file_write+0xa4/0x170
[  243.609709]  vfs_write+0xf0/0x2a4
[  243.613016]  ksys_write+0x68/0xf4
[  243.616323]  __arm64_sys_write+0x1c/0x2c
[  243.620241]  el0_svc_common.constprop.0+0x78/0x1c4
[  243.625022]  do_el0_svc+0x24/0x8c
[  243.628331]  el0_svc+0x14/0x20
[  243.631378]  el0_sync_handler+0xb0/0xb4
[  243.635206]  el0_sync+0x180/0x1c0
[  243.638523] Code: bad PC value
[  243.641573] ---[ end trace 369e4632cb003adc ]---

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux