| 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
| ||
|
Message-ID: <aHt-RLfgVM-HfTh_@kernel.org>
Date: Sat, 19 Jul 2025 14:15:16 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Elena Reshetova <elena.reshetova@...el.com>
Cc: dave.hansen@...el.com, seanjc@...gle.com, kai.huang@...el.com,
mingo@...nel.org, linux-sgx@...r.kernel.org,
linux-kernel@...r.kernel.org, x86@...nel.org,
asit.k.mallick@...el.com, vincent.r.scarlata@...el.com,
chongc@...gle.com, erdemaktas@...gle.com, vannapurve@...gle.com,
bondarn@...gle.com, scott.raynor@...el.com
Subject: Re: [PATCH v8 1/5] x86/sgx: Introduce a counter to count the
sgx_(vepc_)open()
On Tue, Jul 15, 2025 at 03:40:18PM +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. Implement such a counter,
> sgx_usage_count. It will be used by the driver when attempting
> to call EUPDATESVN SGX instruction.
>
> Note: the sgx_inc_usage_count prototype is defined to return
> int for the cleanliness of the follow-up patches. When the
> EUPDATESVN SGX instruction will be enabled in the follow-up patch,
> the sgx_inc_usage_count will start to return int.
>
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@...el.com>
> ---
> arch/x86/kernel/cpu/sgx/driver.c | 22 ++++++++++++++++------
> arch/x86/kernel/cpu/sgx/encl.c | 1 +
> arch/x86/kernel/cpu/sgx/main.c | 14 ++++++++++++++
> arch/x86/kernel/cpu/sgx/sgx.h | 3 +++
> arch/x86/kernel/cpu/sgx/virt.c | 16 ++++++++++++++--
> 5 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..a2994a74bdff 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,9 +19,15 @@ static int sgx_open(struct inode *inode, struct file *file)
> struct sgx_encl *encl;
> int ret;
>
> + ret = sgx_inc_usage_count();
> + if (ret)
> + return ret;
> +
> encl = kzalloc(sizeof(*encl), GFP_KERNEL);
> - if (!encl)
> - return -ENOMEM;
> + if (!encl) {
> + ret = -ENOMEM;
> + goto err_usage_count;
> + }
>
> kref_init(&encl->refcount);
> xa_init(&encl->page_array);
> @@ -31,14 +37,18 @@ static int sgx_open(struct inode *inode, struct file *file)
> spin_lock_init(&encl->mm_lock);
>
> ret = init_srcu_struct(&encl->srcu);
> - if (ret) {
> - kfree(encl);
> - return ret;
> - }
> + if (ret)
> + goto err_encl;
>
> file->private_data = encl;
>
> return 0;
> +
> +err_encl:
> + kfree(encl);
> +err_usage_count:
> + sgx_dec_usage_count();
> + return ret;
> }
>
> static int sgx_release(struct inode *inode, struct file *file)
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..3b54889ae4a4 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -765,6 +765,7 @@ void sgx_encl_release(struct kref *ref)
> WARN_ON_ONCE(encl->secs.epc_page);
>
> kfree(encl);
> + sgx_dec_usage_count();
> }
>
> /*
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 2de01b379aa3..0e75090f93c9 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
> }
> EXPORT_SYMBOL_GPL(sgx_set_attribute);
>
> +/* Counter to count the active SGX users */
> +static int sgx_usage_count;
> +
> +int sgx_inc_usage_count(void)
> +{
> + sgx_usage_count++;
> + return 0;
> +}
> +
> +void sgx_dec_usage_count(void)
> +{
> + sgx_usage_count--;
> +}
> +
> static int __init sgx_init(void)
> {
> int ret;
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..f5940393d9bd 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
> }
> #endif
>
> +int sgx_inc_usage_count(void);
> +void sgx_dec_usage_count(void);
> +
> void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
>
> #endif /* _X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 7aaa3652e31d..6ce908ed51c9 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,22 +255,34 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
> xa_destroy(&vepc->page_array);
> kfree(vepc);
>
> + sgx_dec_usage_count();
> return 0;
> }
>
> static int sgx_vepc_open(struct inode *inode, struct file *file)
> {
> struct sgx_vepc *vepc;
> + int ret;
> +
> + ret = sgx_inc_usage_count();
> + if (ret)
> + return ret;
>
> vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
> - if (!vepc)
> - return -ENOMEM;
> + if (!vepc) {
> + ret = -ENOMEM;
> + goto err_usage_count;
> + }
> mutex_init(&vepc->lock);
> xa_init(&vepc->page_array);
>
> file->private_data = vepc;
>
> return 0;
> +
> +err_usage_count:
> + sgx_dec_usage_count();
> + return ret;
> }
This is essentially a wrapper over pre-existing function. I vote for
renaming the pre-existing function as __sgx_vepc_open() and add then a
new function calling it:
static int sgx_vepc_open(struct inode *inode, struct file *file)
{
int ret;
ret = sgx_inc_usage_count();
if (ret)
return ret;
ret = __sgx_vepc_open(inode, file);
if (ret) {
sgx_dec_usage_count();
return ret;
}
return 0;
}
I think this a factor better standing point also when having to dig
into this later on (easier to see the big picture) as it has clear
split of responsibilities:
1. top layer manages to usage count
2. lower layer allocates vepc structures (which has nothing to do
with the logic of the usage count).
This comment applies also to sgx_open().
>
> static long sgx_vepc_ioctl(struct file *file,
> --
> 2.45.2
>
BR, Jarkko
Powered by blists - more mailing lists