linux-kernel - Re: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and opportunistically call it during first EPC page alloc

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: <aAv445Sr71NUJP1X@google.com>
Date: Fri, 25 Apr 2025 14:04:35 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Elena Reshetova <elena.reshetova@...el.com>, "jarkko@...nel.org" <jarkko@...nel.org>, 
	Kai Huang <kai.huang@...el.com>, 
	"linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>, Vincent Scarlata <vincent.r.scarlata@...el.com>, 
	"x86@...nel.org" <x86@...nel.org>, Vishal Annapurve <vannapurve@...gle.com>, Chong Cai <chongc@...gle.com>, 
	Asit K Mallick <asit.k.mallick@...el.com>, Erdem Aktas <erdemaktas@...gle.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "bondarn@...gle.com" <bondarn@...gle.com>, 
	"dionnaglaze@...gle.com" <dionnaglaze@...gle.com>, Scott Raynor <scott.raynor@...el.com>
Subject: Re: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and
 opportunistically call it during first EPC page alloc

On Fri, Apr 25, 2025, Dave Hansen wrote:
> On 4/25/25 12:29, Sean Christopherson wrote:
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
> >  	xa_destroy(&vepc->page_array);
> >  	kfree(vepc);
> >  
> > +	sgx_dec_usage_count();
> >  	return 0;
> >  }
> 
> ->release() is not close(). Userspace doesn't have control over when
> release() gets called, so it's a poor thing to say: "wait until all SGX
> struct files have been released, then do EUPDATESVN". At least that's
> what folks have always told me when I went poking around the VFS.
> 
> That alone would make this a non-starter.

And what frees all the EPC pages?  Hint: it's starts with an 'r'.

Userspace is going to be waiting on ->release() no matter what.  There's no getting
around that, all enclaves and virtual EPC instances must be fully destroyed to
ensure the EPC is empty.

At least with this approach, userspace can know that the EPC is "busy", whereas
with automatic updates, userspace has zero visibility.  The only breadcrumb it
would get is the SVN, which presumably can only be retrieved from within an encave.
But even if userspace can easily read the SVN, unless userspace has a priori
knowledge of the SVN it expects, userspace has no way of knowing if the SVN isn't
changing because no update was required, or if the SVN isn't changing because
the kernel can't find a window where there's no active EPC page.

Exposing -EBUSY to userspace gives it a variety of options.  E.g. retry N times,
with M delay, and then force a reboot if all else fails.

If we wanted to be more explicit, it would be trivial to add a getter, but IMO
that's completely unnecessary, as it's fully redundant with the errno from the
setter.

E.g.

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index e8fcf032e842..4dc95d59c11f 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -958,14 +958,26 @@ static int sgx_update_svn(const char *buffer, const struct kernel_param *kp)
        }
 }
 
+static int sgx_can_update_svn(char *buffer, const struct kernel_param *kp)
+{
+       if (!sgx_has_eupdatesvn)
+               return sysfs_emit(buffer, "unsupported\n");
+
+       if (atomic_read(&sgx_usage_count))
+               return sysfs_emit(buffer, "busy\n");
+
+       return sysfs_emit(buffer, "ready\n");
+}
+
 static const struct kernel_param_ops sgx_update_svn_ops = {
        .set = sgx_update_svn,
+       .get = sgx_can_update_svn,
 };
 
 #undef MODULE_PARAM_PREFIX
 #define MODULE_PARAM_PREFIX "sgx."
-device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0200);
-__MODULE_PARM_TYPE(update_svn, "bool");
+device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0644);
+__MODULE_PARM_TYPE(update_svn, "int");
 
 static int __init sgx_init(void)
 {