Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

[Maximilian Luz]

On 7/28/22 10:23, Sudeep Holla wrote:
> On Tue, Jul 26, 2022 at 07:01:28PM +0200, Maximilian Luz wrote:
> > On 7/26/22 17:41, Sudeep Holla wrote:
> > > On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
> > > > So ultimately I think it's better to add a DT entry for it.
> > >
> > > I disagree for the reason that once you discover more apps running on the
> > > secure side, you want to add more entries and update DT on the platform
> > > every time you discover some new firmware entity and you wish to interact
> > > with it from the non-secure side.
> >
> > Just as you'll have to add a driver to the kernel and update whatever is
> > probing the TrEE interface and add those strings to that interface. If
> > you then start doing SoC-specific lists, I think you'd be pretty much
> > re-implementing a DT in the kernel driver...
>
> Yes at the cost of DT being dumping ground for all the SoC specific firmware
> crap. Firmware can be and must be discoverable, no point in dumping it in
> DT as it forces DT upgrade every time something changes in the firmware i.e.
> it can go out of sync quite quickly.

I fully agree with you here on the design level. Firmware _should_ be
discoverable. Unfortunately, in this case it really isn't.

Again, in Windows, this information is stored via the Registry and set
when the driver is installed. An example:

    ; UEFIVAR SECURE APP SERVICE
    HKR,%EFIVarService.RegKey%,Enabled,%REG_DWORD%,1
    HKR,%EFIVarService.RegKey%,MajorVersion,%REG_DWORD%,1
    HKR,%EFIVarService.RegKey%,MinorVersion,%REG_DWORD%,0
    
    ; WINSECAPP SECURE APP SERVICE
    HKR,%WinSecAppService.RegKey%,Enabled,%REG_DWORD%,1
    HKR,%WinSecAppService.RegKey%,SecureApp,%REG_DWORD%,1
    HKR,%WinSecAppService.RegKey%,LoadApp,%REG_DWORD%,0
    HKR,%WinSecAppService.RegKey%,AppName,,"qcom.tz.winsecapp"
    HKR,%WinSecAppService.RegKey%,MajorVersion,%REG_DWORD%,1
    HKR,%WinSecAppService.RegKey%,MinorVersion,%REG_DWORD%,0
    HKR,%WinSecAppService.RegKey%,OSDependencies,%REG_MULTI_SZ%,%RpmbOsService%
    
    ; HDCP v2.2 SECURE APP SERVICE
    HKR,%Hdcp2p2Service.RegKey%,Enabled,%REG_DWORD%,1
    HKR,%Hdcp2p2Service.RegKey%,SecureApp,%REG_DWORD%,1
    HKR,%Hdcp2p2Service.RegKey%,LoadApp,%REG_DWORD%,1
    HKR,%Hdcp2p2Service.RegKey%,AppName,,"qcom.tz.hdcp2p2"
    HKR,%Hdcp2p2Service.RegKey%,FileName,,"hdcp2p2.mbn"
    HKR,%Hdcp2p2Service.RegKey%,MajorVersion,%REG_DWORD%,1
    HKR,%Hdcp2p2Service.RegKey%,MinorVersion,%REG_DWORD%,0
    HKR,%Hdcp2p2Service.RegKey%,OSDependencies,%REG_MULTI_SZ%,%RpmbOsService%,%TzAppsOsService%

The '.RegKey' contains a GUID that specifies the _driver_ interface that
is registered by the driver to the kernel (i.e. is not related to the
specific firmware and firmware version), e.g. [1]. For uefisecapp, the
driver also maps this GUID to the name-string.

[1]: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/km/treevariableservice.h#L35

> > I don't quite understand why this is a problem. I think per device,
> > there's a reasonably limited set of apps that we would want to interact
> > with from the kernel. And for one single device, that set doesn't change
> > over time. So what's the difference to, say, an I2C device?
>
> As I said we don't want DT to be dumping ground for all the not well designed
> firmware interface. The whole point of firmware being another piece of
> software that can be change unlike hardware makes it fragile to present any
> more that what you need in the DT. I see this as one of the example.

I can see your point. But this interface has apparently been around
since at least sdm850 (e.g. Lenovo C630) and hasn't changed. As I've
argued elsewhere: All parties involved have a vested interest that this
interface doesn't change in a breaking way. The interface is modeled
similar to syscalls, so I very much expect them to extend it if needed,
instead of changing/breaking it.

Sure, it _could_ be changed in a breaking way. But again, I believe that
to be _very_ unlikely.

> Anyways I don't have the final say, I leave it to the DT maintainers.
>
> > > As along as get this application ID can handle any random name, I prefer
> > > to use that as the discover mechanism and not have this DT.
> >
> > Apart from the above, some apps must also be loaded from the system. And
> > those you can't detect: If an app isn't running, it doesn't have an ID
> > (uefisecapp and the tpm app are loaded by the firmware at boot). Those
> > are mostly vendor-specific things as far as I can tell, or HDCP stuff.
> > So you'd need to specify those as firmware somehow, and since (as far as
> > I can tell) those are signed specifically by/for that vendor and
> > potentially device (similar to the GPU zap shader or remoteproc
> > firmware), you'll need to use per-device paths.
>
> Sounds to me like more can be pushed to user space as it gets loaded at
> runtime.

If we have user-space available at the time when these things should be
loaded or if they are more or less optional, sure.

> > That means you either hard-code them in the driver and have a compatible
> > per model, do DMI matching, or something similar (again, essentially
> > baking DTs into the kernel driver...), or just store them in the DT
> > (like we already do for GPU/remoteprocs). While you could hard-code some
> > known loaded-by-firmware apps and use the DT for others, I think we
> > should keep everything in the same place.
>
> Worst case I am fine with that as this needs to be one of and future
> platforms must get their act right in designing their f/w interface.

Again, I fully agree with you that this situation shouldn't exist. But
reality is sadly different.

Regards,
Max