linux-kernel - Re: [PATCH v1 1/1] tee: optee: expose OS revision via sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d4b86b19-0274-45e6-89e3-0ad927fe5cdd@oss.qualcomm.com>
Date: Sat, 22 Nov 2025 13:06:12 +0530
From: Harshal Dev <harshal.dev@....qualcomm.com>
To: Wei Ming Chen <jj251510319013@...il.com>, linux-kernel@...r.kernel.org
Cc: sumit.garg@...nel.org, op-tee@...ts.trustedfirmware.org,
        Aristo Chen <aristo.chen@...onical.com>
Subject: Re: [PATCH v1 1/1] tee: optee: expose OS revision via sysfs

Hi Aristo,

On 11/21/2025 7:42 PM, Wei Ming Chen wrote:
> From: Aristo Chen <aristo.chen@...onical.com>
> 
> Today the only way to read the OP-TEE OS version is from dmesg/journal
> logs, which can be lost as buffers roll over. Add a minimal
> optee_version_info (major/minor/build) and get_optee_revision hook,
> collect the OS revision in both SMC and FF-A ABIs, and publish
> /sys/class/tee/tee*/optee_os_revision for a stable userspace readout.
> 
> Signed-off-by: Aristo Chen <aristo.chen@...onical.com>
> ---
>  drivers/tee/optee/ffa_abi.c       | 27 +++++++++++-
>  drivers/tee/optee/optee_private.h |  1 +
>  drivers/tee/optee/smc_abi.c       | 27 +++++++++++-
>  drivers/tee/tee_core.c            | 73 ++++++++++++++++++++++++++++++-
>  include/linux/tee_core.h          |  2 +
>  include/linux/tee_drv.h           | 12 +++++
>  6 files changed, 137 insertions(+), 5 deletions(-)

[..]

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index d65d47cc154e..dc23058e7be6 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -1141,12 +1141,83 @@ static ssize_t implementation_id_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(implementation_id);
>  
> +static int tee_get_optee_revision(struct tee_device *teedev,
> +				  struct optee_version_info *ver_info)
> +{
> +	if (!teedev->desc->ops->get_optee_revision)
> +		return -ENODEV;
> +
> +	return teedev->desc->ops->get_optee_revision(teedev, ver_info);
> +}
> +

I don't think implementing functions with a TEE back-end driver specific name and implementation
is a good idea in TEE core.

> +static bool tee_is_optee(struct tee_device *teedev)
> +{
> +	struct tee_ioctl_version_data vers;
> +
> +	teedev->desc->ops->get_version(teedev, &vers);
> +
> +	return vers.impl_id == TEE_IMPL_ID_OPTEE;
> +}
> +
> +static ssize_t optee_os_revision_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct tee_device *teedev = container_of(dev, struct tee_device, dev);
> +	struct optee_version_info ver_info;
> +	int ret;
> +
> +	if (!tee_is_optee(teedev))
> +		return -ENODEV;
> +
> +	ret = tee_get_optee_revision(teedev, &ver_info);
> +	if (ret)
> +		return ret;
> +
> +	if (ver_info.os_build_id)
> +		return sysfs_emit(buf, "%u.%u (%08x)\n", ver_info.os_major,
> +				  ver_info.os_minor, ver_info.os_build_id);
> +
> +	return sysfs_emit(buf, "%u.%u\n", ver_info.os_major,
> +			  ver_info.os_minor);
> +}

[..]

> diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h
> index 1f3e5dad6d0d..4bd9b6b191c9 100644
> --- a/include/linux/tee_core.h
> +++ b/include/linux/tee_core.h
> @@ -98,6 +98,8 @@ struct tee_device {
>  struct tee_driver_ops {
>  	void (*get_version)(struct tee_device *teedev,
>  			    struct tee_ioctl_version_data *vers);
> +	int (*get_optee_revision)(struct tee_device *teedev,
> +				  struct optee_version_info *vers);

I think it would be better if this patchset could be made OPTEE agnostic. After-all,
the callbacks defined in tee_driver_ops are supposed to be implemented by each TEE
back-end driver as per their implementation.

>  	int (*open)(struct tee_context *ctx);
>  	void (*close_context)(struct tee_context *ctx);
>  	void (*release)(struct tee_context *ctx);
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 88a6f9697c89..64a2fea11cb9 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -20,6 +20,18 @@
>  
>  struct tee_device;
>  
> +/**
> + * struct optee_version_info - OP-TEE version information
> + * @os_major:		OS major version
> + * @os_minor:		OS minor version
> + * @os_build_id:	OS build identifier (0 if unspecified)
> + */
> +struct optee_version_info {
> +	u32 os_major;
> +	u32 os_minor;
> +	u32 os_build_id;
> +};
> +

It is not entirely clear from the structure what kind of version information for
OPTEE is being provided. I see from the implementation that when FFA is enabled,
this provides FFA version implemented by OPTEE but when FFA is not used, it provides
OPTEE OS version. This can be confusing for a user trying to report an OPTEE 'version'.

>  /**
>   * struct tee_context - driver specific context on file pointer data
>   * @teedev:	pointer to this drivers struct tee_device

Thanks
Harshal