[PATCH v19 17/27] x86/sgx: Add provisioning

[PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

In order to provide a mechanism for devilering provisoning rights:

1. Add a new file to the securityfs file called sgx/provision that works
   as a token for allowing an enclave to have the provisioning privileges.
2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
   following data structure:

   struct sgx_enclave_set_attribute {
           __u64 addr;
           __u64 token_fd;
   };

A daemon could sit on top of sgx/provision and send a file descriptor of
this file to a process that needs to be able to provision enclaves.

The way this API is used is more or less straight-forward. Lets assume that
dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
PROVISIONKEY attribute by

params.addr = <enclave address>;
params.token_fd = prov_fd;

ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, &params);

Cc: James Morris <jmorris@namei.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        | 13 +++++++
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/driver/main.c  | 47 ++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index aadf9c76e360..150a784db395 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -16,6 +16,8 @@
 	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
 #define SGX_IOC_ENCLAVE_INIT \
 	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
+	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
 
 /* IOCTL return values */
 #define SGX_POWER_LOST_ENCLAVE		0x40000000
@@ -56,4 +58,15 @@ struct sgx_enclave_init {
 	__u64	sigstruct;
 };
 
+/**
+ * struct sgx_enclave_set_attribute - parameter structure for the
+ *				      %SGX_IOC_ENCLAVE_INIT ioctl
+ * @addr:		address within the ELRANGE
+ * @attribute_fd:	file handle of the attribute file in the securityfs
+ */
+struct sgx_enclave_set_attribute {
+	__u64	addr;
+	__u64	attribute_fd;
+};
+
 #endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 4b9a91b53b50..5d85bd3f7876 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
 	return ret;
 }
 
+/**
+ * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
+ * @filep:	open file to /dev/sgx
+ * @cmd:	the command value
+ * @arg:	pointer to a struct sgx_enclave_set_attribute instance
+ *
+ * Sets an attribute matching the attribute file that is pointed by the
+ * parameter structure field attribute_fd.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
+					  unsigned long arg)
+{
+	struct sgx_enclave_set_attribute *params = (void *)arg;
+	struct file *attribute_file;
+	struct sgx_encl *encl;
+	int ret;
+
+	attribute_file = fget(params->attribute_fd);
+	if (!attribute_file->f_op)
+		return -EINVAL;
+
+	if (attribute_file->f_op != &sgx_fs_provision_fops) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = sgx_encl_get(params->addr, &encl);
+	if (ret)
+		goto out;
+
+	encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
+	kref_put(&encl->refcount, sgx_encl_release);
+
+out:
+	fput(attribute_file);
+	return ret;
+}
+
 typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
 			  unsigned long arg);
 
@@ -778,6 +818,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	case SGX_IOC_ENCLAVE_INIT:
 		handler = sgx_ioc_enclave_init;
 		break;
+	case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
+		handler = sgx_ioc_enclave_set_attribute;
+		break;
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 16f36cd0af04..9a5360dcad98 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -22,6 +22,11 @@ u64 sgx_attributes_reserved_mask;
 u64 sgx_xfrm_reserved_mask = ~0x3;
 u32 sgx_xsave_size_tbl[64];
 
+const struct file_operations sgx_fs_provision_fops;
+
+static struct dentry *sgx_fs;
+static struct dentry *sgx_fs_provision;
+
 #ifdef CONFIG_COMPAT
 static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
 			      unsigned long arg)
@@ -147,6 +152,40 @@ static struct sgx_dev_ctx *sgxm_dev_ctx_alloc(struct device *parent)
 	return ctx;
 }
 
+static int sgx_fs_init(struct device *dev)
+{
+	int ret;
+
+	sgx_fs = securityfs_create_dir(dev_name(dev), NULL);
+	if (IS_ERR(sgx_fs)) {
+		ret = PTR_ERR(sgx_fs);
+		goto err_sgx_fs;
+	}
+
+	sgx_fs_provision = securityfs_create_file("provision", 0600, sgx_fs,
+						  NULL, &sgx_fs_provision_fops);
+	if (IS_ERR(sgx_fs)) {
+		ret = PTR_ERR(sgx_fs_provision);
+		goto err_sgx_fs_provision;
+	}
+
+	return 0;
+
+err_sgx_fs_provision:
+	securityfs_remove(sgx_fs);
+	sgx_fs_provision = NULL;
+
+err_sgx_fs:
+	sgx_fs = NULL;
+	return ret;
+}
+
+static void sgx_fs_remove(void)
+{
+	securityfs_remove(sgx_fs_provision);
+	securityfs_remove(sgx_fs);
+}
+
 static int sgx_dev_init(struct device *parent)
 {
 	struct sgx_dev_ctx *sgx_dev;
@@ -190,6 +229,10 @@ static int sgx_dev_init(struct device *parent)
 	if (!sgx_encl_wq)
 		return -ENOMEM;
 
+	ret = sgx_fs_init(&sgx_dev->ctrl_dev);
+	if (ret)
+		goto err_fs_init;
+
 	ret = cdev_device_add(&sgx_dev->ctrl_cdev, &sgx_dev->ctrl_dev);
 	if (ret)
 		goto err_device_add;
@@ -197,6 +240,9 @@ static int sgx_dev_init(struct device *parent)
 	return 0;
 
 err_device_add:
+	sgx_fs_remove();
+
+err_fs_init:
 	destroy_workqueue(sgx_encl_wq);
 	return ret;
 }
@@ -220,6 +266,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
 {
 	struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
 
+	sgx_fs_remove();
 	cdev_device_del(&ctx->ctrl_cdev, &ctx->ctrl_dev);
 	destroy_workqueue(sgx_encl_wq);
 
-- 
2.19.1

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Sean Christopherson]

On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> In order to provide a mechanism for devilering provisoning rights:
> 
> 1. Add a new file to the securityfs file called sgx/provision that works
>    as a token for allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
>    following data structure:
> 
>    struct sgx_enclave_set_attribute {
>            __u64 addr;
>            __u64 token_fd;
>    };
> 
> A daemon could sit on top of sgx/provision and send a file descriptor of
> this file to a process that needs to be able to provision enclaves.
> 
> The way this API is used is more or less straight-forward. Lets assume that
> dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> PROVISIONKEY attribute by
> 
> params.addr = <enclave address>;
> params.token_fd = prov_fd;
> 
> ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, &params);
> 
> Cc: James Morris <jmorris@namei.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h        | 13 +++++++
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/driver/main.c  | 47 ++++++++++++++++++++++++++
>  3 files changed, 103 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index aadf9c76e360..150a784db395 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -16,6 +16,8 @@
>  	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
>  #define SGX_IOC_ENCLAVE_INIT \
>  	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> +	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>  
>  /* IOCTL return values */
>  #define SGX_POWER_LOST_ENCLAVE		0x40000000
> @@ -56,4 +58,15 @@ struct sgx_enclave_init {
>  	__u64	sigstruct;
>  };
>  
> +/**
> + * struct sgx_enclave_set_attribute - parameter structure for the
> + *				      %SGX_IOC_ENCLAVE_INIT ioctl
> + * @addr:		address within the ELRANGE
> + * @attribute_fd:	file handle of the attribute file in the securityfs
> + */
> +struct sgx_enclave_set_attribute {
> +	__u64	addr;
> +	__u64	attribute_fd;
> +};
> +
>  #endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 4b9a91b53b50..5d85bd3f7876 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
>  	return ret;
>  }
>  
> +/**
> + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> + * @filep:	open file to /dev/sgx
> + * @cmd:	the command value
> + * @arg:	pointer to a struct sgx_enclave_set_attribute instance
> + *
> + * Sets an attribute matching the attribute file that is pointed by the
> + * parameter structure field attribute_fd.

With the @data change (see below), this becomes something like:

 * Allow the enclave to request the attribute managed by the SGX security file
 * pointed at by the parameter structure field attribute_fd.

> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
> +					  unsigned long arg)
> +{
> +	struct sgx_enclave_set_attribute *params = (void *)arg;
> +	struct file *attribute_file;
> +	struct sgx_encl *encl;
> +	int ret;
> +
> +	attribute_file = fget(params->attribute_fd);
> +	if (!attribute_file->f_op)

This should be:

	if (!attribute_file)
		return -EINVAL;

> +		return -EINVAL;
> +
> +	if (attribute_file->f_op != &sgx_fs_provision_fops) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = sgx_encl_get(params->addr, &encl);
> +	if (ret)
> +		goto out;
> +
> +	encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;

A cleanr approach would be to pass SGX_ATTR_PROVISIONKEY via @data to
securityfs_create_file().  Then you don't need to define dummy file_ops
for each file, i.e. a generic sgx_sec_fs_ops would suffice for the above
check.  And you don't have this weird hardcoding of the provision bit.

E.g.:

	if (attribute_file->f_op != &sgx_sec_fs_fops) {
		ret = -EINVAL;
		goto out;
	}

	ret = sgx_encl_get(params->addr, &encl);
	if (ret)
		goto out;

	encl->allowed_attributes |= (u64)attribute_file->private_data;

Since SGX doesn't support 32-bit builds we don't even need to worry about
the (very distant) future where SGX defines bits in the 63:32 range.


> +	kref_put(&encl->refcount, sgx_encl_release);
> +
> +out:
> +	fput(attribute_file);
> +	return ret;
> +}
> +
>  typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
>  			  unsigned long arg);
>  
> @@ -778,6 +818,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>  	case SGX_IOC_ENCLAVE_INIT:
>  		handler = sgx_ioc_enclave_init;
>  		break;
> +	case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
> +		handler = sgx_ioc_enclave_set_attribute;
> +		break;
>  	default:
>  		return -ENOIOCTLCMD;
>  	}
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 16f36cd0af04..9a5360dcad98 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -22,6 +22,11 @@ u64 sgx_attributes_reserved_mask;
>  u64 sgx_xfrm_reserved_mask = ~0x3;
>  u32 sgx_xsave_size_tbl[64];
>  
> +const struct file_operations sgx_fs_provision_fops;
> +
> +static struct dentry *sgx_fs;
> +static struct dentry *sgx_fs_provision;
> +
>  #ifdef CONFIG_COMPAT
>  static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
>  			      unsigned long arg)
> @@ -147,6 +152,40 @@ static struct sgx_dev_ctx *sgxm_dev_ctx_alloc(struct device *parent)
>  	return ctx;
>  }
>  
> +static int sgx_fs_init(struct device *dev)
> +{
> +	int ret;
> +
> +	sgx_fs = securityfs_create_dir(dev_name(dev), NULL);
> +	if (IS_ERR(sgx_fs)) {
> +		ret = PTR_ERR(sgx_fs);
> +		goto err_sgx_fs;
> +	}
> +
> +	sgx_fs_provision = securityfs_create_file("provision", 0600, sgx_fs,
> +						  NULL, &sgx_fs_provision_fops);

Per above, pass SGX_ATTR_PROVISIONKEY instead of NULL.

> +	if (IS_ERR(sgx_fs)) {
> +		ret = PTR_ERR(sgx_fs_provision);
> +		goto err_sgx_fs_provision;
> +	}
> +
> +	return 0;
> +
> +err_sgx_fs_provision:
> +	securityfs_remove(sgx_fs);
> +	sgx_fs_provision = NULL;
> +
> +err_sgx_fs:
> +	sgx_fs = NULL;
> +	return ret;
> +}
> +
> +static void sgx_fs_remove(void)
> +{
> +	securityfs_remove(sgx_fs_provision);
> +	securityfs_remove(sgx_fs);
> +}
> +
>  static int sgx_dev_init(struct device *parent)
>  {
>  	struct sgx_dev_ctx *sgx_dev;
> @@ -190,6 +229,10 @@ static int sgx_dev_init(struct device *parent)
>  	if (!sgx_encl_wq)
>  		return -ENOMEM;
>  
> +	ret = sgx_fs_init(&sgx_dev->ctrl_dev);
> +	if (ret)
> +		goto err_fs_init;
> +
>  	ret = cdev_device_add(&sgx_dev->ctrl_cdev, &sgx_dev->ctrl_dev);
>  	if (ret)
>  		goto err_device_add;
> @@ -197,6 +240,9 @@ static int sgx_dev_init(struct device *parent)
>  	return 0;
>  
>  err_device_add:
> +	sgx_fs_remove();
> +
> +err_fs_init:
>  	destroy_workqueue(sgx_encl_wq);
>  	return ret;
>  }
> @@ -220,6 +266,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
>  {
>  	struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
>  
> +	sgx_fs_remove();
>  	cdev_device_del(&ctx->ctrl_cdev, &ctx->ctrl_dev);
>  	destroy_workqueue(sgx_encl_wq);
>  
> -- 
> 2.19.1
> 

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Huang, Kai]

On Tue, 2019-03-19 at 13:09 -0700, Sean Christopherson wrote:
> On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > In order to provide a mechanism for devilering provisoning rights:
> > 
> > 1. Add a new file to the securityfs file called sgx/provision that works
> >    as a token for allowing an enclave to have the provisioning privileges.
> > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> >    following data structure:
> > 
> >    struct sgx_enclave_set_attribute {
> >            __u64 addr;
> >            __u64 token_fd;
> >    };

Would you elaborate why the name is "token_fd"? I think *token* in SGX has more specific meaning?

> > 
> > A daemon could sit on top of sgx/provision and send a file descriptor of
> > this file to a process that needs to be able to provision enclaves.
> > 
> > The way this API is used is more or less straight-forward. Lets assume that
> > dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> > You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> > PROVISIONKEY attribute by
> > 
> > params.addr = <enclave address>;
> > params.token_fd = prov_fd;
> > 
> > ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, &params);
> > 
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Serge E. Hallyn <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Suggested-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/include/uapi/asm/sgx.h        | 13 +++++++
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
> >  arch/x86/kernel/cpu/sgx/driver/main.c  | 47 ++++++++++++++++++++++++++
> >  3 files changed, 103 insertions(+)
> > 
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index aadf9c76e360..150a784db395 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -16,6 +16,8 @@
> >  	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> >  #define SGX_IOC_ENCLAVE_INIT \
> >  	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> > +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> > +	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
> >  
> >  /* IOCTL return values */
> >  #define SGX_POWER_LOST_ENCLAVE		0x40000000
> > @@ -56,4 +58,15 @@ struct sgx_enclave_init {
> >  	__u64	sigstruct;
> >  };
> >  
> > +/**
> > + * struct sgx_enclave_set_attribute - parameter structure for the
> > + *				      %SGX_IOC_ENCLAVE_INIT ioctl
> > + * @addr:		address within the ELRANGE
> > + * @attribute_fd:	file handle of the attribute file in the securityfs
> > + */
> > +struct sgx_enclave_set_attribute {
> > +	__u64	addr;
> > +	__u64	attribute_fd;
> > +};
> > +
> >  #endif /* _UAPI_ASM_X86_SGX_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index 4b9a91b53b50..5d85bd3f7876 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> > + * @filep:	open file to /dev/sgx
> > + * @cmd:	the command value
> > + * @arg:	pointer to a struct sgx_enclave_set_attribute instance
> > + *
> > + * Sets an attribute matching the attribute file that is pointed by the
> > + * parameter structure field attribute_fd.
> 
> With the @data change (see below), this becomes something like:
> 
>  * Allow the enclave to request the attribute managed by the SGX security file
>  * pointed at by the parameter structure field attribute_fd.
> 
> > + *
> > + * Return: 0 on success, -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
> > +					  unsigned long arg)
> > +{
> > +	struct sgx_enclave_set_attribute *params = (void *)arg;
> > +	struct file *attribute_file;
> > +	struct sgx_encl *encl;
> > +	int ret;
> > +
> > +	attribute_file = fget(params->attribute_fd);
> > +	if (!attribute_file->f_op)
> 
> This should be:
> 
> 	if (!attribute_file)
> 		return -EINVAL;
> 
> > +		return -EINVAL;
> > +
> > +	if (attribute_file->f_op != &sgx_fs_provision_fops) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	ret = sgx_encl_get(params->addr, &encl);
> > +	if (ret)
> > +		goto out;
> > +
> > +	encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
> 
> A cleanr approach would be to pass SGX_ATTR_PROVISIONKEY via @data to
> securityfs_create_file().  Then you don't need to define dummy file_ops
> for each file, i.e. a generic sgx_sec_fs_ops would suffice for the above
> check.  And you don't have this weird hardcoding of the provision bit.
> 
> E.g.:
> 
> 	if (attribute_file->f_op != &sgx_sec_fs_fops) {
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 
> 	ret = sgx_encl_get(params->addr, &encl);
> 	if (ret)
> 		goto out;
> 
> 	encl->allowed_attributes |= (u64)attribute_file->private_data;
> 
> Since SGX doesn't support 32-bit builds we don't even need to worry about
> the (very distant) future where SGX defines bits in the 63:32 range.
> 

Agree with Sean that passing SGX_ATTR_PROVISIONKEY via @data is more cleaner.

Thanks,
-Kai

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Thu, Mar 21, 2019 at 02:08:36AM +0000, Huang, Kai wrote:
> On Tue, 2019-03-19 at 13:09 -0700, Sean Christopherson wrote:
> > On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > > In order to provide a mechanism for devilering provisoning rights:
> > > 
> > > 1. Add a new file to the securityfs file called sgx/provision that works
> > >    as a token for allowing an enclave to have the provisioning privileges.
> > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > >    following data structure:
> > > 
> > >    struct sgx_enclave_set_attribute {
> > >            __u64 addr;
> > >            __u64 token_fd;
> > >    };
> 
> Would you elaborate why the name is "token_fd"? I think *token* in SGX
> has more specific meaning?

I'm open for other names.

/Jarkko

RE: [PATCH v19 17/27] x86/sgx: Add provisioning

[Huang, Kai]

> On Thu, Mar 21, 2019 at 02:08:36AM +0000, Huang, Kai wrote:
> > On Tue, 2019-03-19 at 13:09 -0700, Sean Christopherson wrote:
> > > On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > > > In order to provide a mechanism for devilering provisoning rights:
> > > >
> > > > 1. Add a new file to the securityfs file called sgx/provision that works
> > > >    as a token for allowing an enclave to have the provisioning privileges.
> > > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that
> accepts the
> > > >    following data structure:
> > > >
> > > >    struct sgx_enclave_set_attribute {
> > > >            __u64 addr;
> > > >            __u64 token_fd;
> > > >    };
> >
> > Would you elaborate why the name is "token_fd"? I think *token* in SGX
> > has more specific meaning?
> 
> I'm open for other names.

How about attribute_fd, which is consistent with your IOCTL?

Thanks,
-Kai

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Thu, Mar 21, 2019 at 09:41:37PM +0000, Huang, Kai wrote:
> > On Thu, Mar 21, 2019 at 02:08:36AM +0000, Huang, Kai wrote:
> > > On Tue, 2019-03-19 at 13:09 -0700, Sean Christopherson wrote:
> > > > On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > > > > In order to provide a mechanism for devilering provisoning rights:
> > > > >
> > > > > 1. Add a new file to the securityfs file called sgx/provision that works
> > > > >    as a token for allowing an enclave to have the provisioning privileges.
> > > > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that
> > accepts the
> > > > >    following data structure:
> > > > >
> > > > >    struct sgx_enclave_set_attribute {
> > > > >            __u64 addr;
> > > > >            __u64 token_fd;
> > > > >    };
> > >
> > > Would you elaborate why the name is "token_fd"? I think *token* in SGX
> > > has more specific meaning?
> > 
> > I'm open for other names.
> 
> How about attribute_fd, which is consistent with your IOCTL?

I'm cool with that name! Thanks.

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Tue, Mar 19, 2019 at 01:09:12PM -0700, Sean Christopherson wrote:
> On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > In order to provide a mechanism for devilering provisoning rights:
> > 
> > 1. Add a new file to the securityfs file called sgx/provision that works
> >    as a token for allowing an enclave to have the provisioning privileges.
> > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> >    following data structure:
> > 
> >    struct sgx_enclave_set_attribute {
> >            __u64 addr;
> >            __u64 token_fd;
> >    };
> > 
> > A daemon could sit on top of sgx/provision and send a file descriptor of
> > this file to a process that needs to be able to provision enclaves.
> > 
> > The way this API is used is more or less straight-forward. Lets assume that
> > dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> > You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> > PROVISIONKEY attribute by
> > 
> > params.addr = <enclave address>;
> > params.token_fd = prov_fd;
> > 
> > ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, &params);
> > 
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Serge E. Hallyn <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Suggested-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/include/uapi/asm/sgx.h        | 13 +++++++
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
> >  arch/x86/kernel/cpu/sgx/driver/main.c  | 47 ++++++++++++++++++++++++++
> >  3 files changed, 103 insertions(+)
> > 
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index aadf9c76e360..150a784db395 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -16,6 +16,8 @@
> >  	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> >  #define SGX_IOC_ENCLAVE_INIT \
> >  	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> > +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> > +	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
> >  
> >  /* IOCTL return values */
> >  #define SGX_POWER_LOST_ENCLAVE		0x40000000
> > @@ -56,4 +58,15 @@ struct sgx_enclave_init {
> >  	__u64	sigstruct;
> >  };
> >  
> > +/**
> > + * struct sgx_enclave_set_attribute - parameter structure for the
> > + *				      %SGX_IOC_ENCLAVE_INIT ioctl
> > + * @addr:		address within the ELRANGE
> > + * @attribute_fd:	file handle of the attribute file in the securityfs
> > + */
> > +struct sgx_enclave_set_attribute {
> > +	__u64	addr;
> > +	__u64	attribute_fd;
> > +};
> > +
> >  #endif /* _UAPI_ASM_X86_SGX_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index 4b9a91b53b50..5d85bd3f7876 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> > + * @filep:	open file to /dev/sgx
> > + * @cmd:	the command value
> > + * @arg:	pointer to a struct sgx_enclave_set_attribute instance
> > + *
> > + * Sets an attribute matching the attribute file that is pointed by the
> > + * parameter structure field attribute_fd.
> 
> With the @data change (see below), this becomes something like:
> 
>  * Allow the enclave to request the attribute managed by the SGX security file
>  * pointed at by the parameter structure field attribute_fd.

I see your point but the current implementation is just a tiny bit
simpler and right now we have one single file.

But I would do to fix this patch right now is to rename
sgx_fs_provision_ops as sgx_fs_ops to point out that only single fops
are required.

The current implementation is "good enough" for handling the
provisioning key.

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Nathaniel McCallum]

On Sun, Mar 17, 2019 at 5:18 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> In order to provide a mechanism for devilering provisoning rights:

I'm not sure what nefarious spirits have to do with this patch.
Perhaps you meant "delivering provisioning"? ;)

> 1. Add a new file to the securityfs file called sgx/provision that works
>    as a token for allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
>    following data structure:
>
>    struct sgx_enclave_set_attribute {
>            __u64 addr;
>            __u64 token_fd;
>    };
>
> A daemon could sit on top of sgx/provision and send a file descriptor of
> this file to a process that needs to be able to provision enclaves.
>
> The way this API is used is more or less straight-forward. Lets assume that
> dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> PROVISIONKEY attribute by
>
> params.addr = <enclave address>;
> params.token_fd = prov_fd;
>
> ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, &params);
>
> Cc: James Morris <jmorris@namei.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h        | 13 +++++++
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/driver/main.c  | 47 ++++++++++++++++++++++++++
>  3 files changed, 103 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index aadf9c76e360..150a784db395 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -16,6 +16,8 @@
>         _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
>  #define SGX_IOC_ENCLAVE_INIT \
>         _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> +       _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>
>  /* IOCTL return values */
>  #define SGX_POWER_LOST_ENCLAVE         0x40000000
> @@ -56,4 +58,15 @@ struct sgx_enclave_init {
>         __u64   sigstruct;
>  };
>
> +/**
> + * struct sgx_enclave_set_attribute - parameter structure for the
> + *                                   %SGX_IOC_ENCLAVE_INIT ioctl
> + * @addr:              address within the ELRANGE
> + * @attribute_fd:      file handle of the attribute file in the securityfs
> + */
> +struct sgx_enclave_set_attribute {
> +       __u64   addr;
> +       __u64   attribute_fd;
> +};
> +
>  #endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 4b9a91b53b50..5d85bd3f7876 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
>         return ret;
>  }
>
> +/**
> + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> + * @filep:     open file to /dev/sgx
> + * @cmd:       the command value
> + * @arg:       pointer to a struct sgx_enclave_set_attribute instance
> + *
> + * Sets an attribute matching the attribute file that is pointed by the
> + * parameter structure field attribute_fd.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
> +                                         unsigned long arg)
> +{
> +       struct sgx_enclave_set_attribute *params = (void *)arg;
> +       struct file *attribute_file;
> +       struct sgx_encl *encl;
> +       int ret;
> +
> +       attribute_file = fget(params->attribute_fd);
> +       if (!attribute_file->f_op)
> +               return -EINVAL;
> +
> +       if (attribute_file->f_op != &sgx_fs_provision_fops) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = sgx_encl_get(params->addr, &encl);
> +       if (ret)
> +               goto out;
> +
> +       encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
> +       kref_put(&encl->refcount, sgx_encl_release);
> +
> +out:
> +       fput(attribute_file);
> +       return ret;
> +}
> +
>  typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
>                           unsigned long arg);
>
> @@ -778,6 +818,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>         case SGX_IOC_ENCLAVE_INIT:
>                 handler = sgx_ioc_enclave_init;
>                 break;
> +       case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
> +               handler = sgx_ioc_enclave_set_attribute;
> +               break;
>         default:
>                 return -ENOIOCTLCMD;
>         }
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 16f36cd0af04..9a5360dcad98 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -22,6 +22,11 @@ u64 sgx_attributes_reserved_mask;
>  u64 sgx_xfrm_reserved_mask = ~0x3;
>  u32 sgx_xsave_size_tbl[64];
>
> +const struct file_operations sgx_fs_provision_fops;
> +
> +static struct dentry *sgx_fs;
> +static struct dentry *sgx_fs_provision;
> +
>  #ifdef CONFIG_COMPAT
>  static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
>                               unsigned long arg)
> @@ -147,6 +152,40 @@ static struct sgx_dev_ctx *sgxm_dev_ctx_alloc(struct device *parent)
>         return ctx;
>  }
>
> +static int sgx_fs_init(struct device *dev)
> +{
> +       int ret;
> +
> +       sgx_fs = securityfs_create_dir(dev_name(dev), NULL);
> +       if (IS_ERR(sgx_fs)) {
> +               ret = PTR_ERR(sgx_fs);
> +               goto err_sgx_fs;
> +       }
> +
> +       sgx_fs_provision = securityfs_create_file("provision", 0600, sgx_fs,
> +                                                 NULL, &sgx_fs_provision_fops);
> +       if (IS_ERR(sgx_fs)) {
> +               ret = PTR_ERR(sgx_fs_provision);
> +               goto err_sgx_fs_provision;
> +       }
> +
> +       return 0;
> +
> +err_sgx_fs_provision:
> +       securityfs_remove(sgx_fs);
> +       sgx_fs_provision = NULL;
> +
> +err_sgx_fs:
> +       sgx_fs = NULL;
> +       return ret;
> +}
> +
> +static void sgx_fs_remove(void)
> +{
> +       securityfs_remove(sgx_fs_provision);
> +       securityfs_remove(sgx_fs);
> +}
> +
>  static int sgx_dev_init(struct device *parent)
>  {
>         struct sgx_dev_ctx *sgx_dev;
> @@ -190,6 +229,10 @@ static int sgx_dev_init(struct device *parent)
>         if (!sgx_encl_wq)
>                 return -ENOMEM;
>
> +       ret = sgx_fs_init(&sgx_dev->ctrl_dev);
> +       if (ret)
> +               goto err_fs_init;
> +
>         ret = cdev_device_add(&sgx_dev->ctrl_cdev, &sgx_dev->ctrl_dev);
>         if (ret)
>                 goto err_device_add;
> @@ -197,6 +240,9 @@ static int sgx_dev_init(struct device *parent)
>         return 0;
>
>  err_device_add:
> +       sgx_fs_remove();
> +
> +err_fs_init:
>         destroy_workqueue(sgx_encl_wq);
>         return ret;
>  }
> @@ -220,6 +266,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
>  {
>         struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
>
> +       sgx_fs_remove();
>         cdev_device_del(&ctx->ctrl_cdev, &ctx->ctrl_dev);
>         destroy_workqueue(sgx_encl_wq);
>
> --
> 2.19.1
>

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Thu, Mar 21, 2019 at 10:38:32AM -0400, Nathaniel McCallum wrote:
> On Sun, Mar 17, 2019 at 5:18 PM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > In order to provide a mechanism for devilering provisoning rights:
>
> I'm not sure what nefarious spirits have to do with this patch.
> Perhaps you meant "delivering provisioning"? ;)

Oops, yeah, maybe summoning won't this time :-)

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Andy Lutomirski]

On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> In order to provide a mechanism for devilering provisoning rights:
>
> 1. Add a new file to the securityfs file called sgx/provision that works
>    as a token for allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
>    following data structure:
>
>    struct sgx_enclave_set_attribute {
>            __u64 addr;
>            __u64 token_fd;
>    };

Here's a potential issue:

For container use, is it reasonable for a container manager to
bind-mount a file into securityfs?  Or would something in /dev make
this easier?

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Thu, Mar 21, 2019 at 09:50:41AM -0700, Andy Lutomirski wrote:
> On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > In order to provide a mechanism for devilering provisoning rights:
> >
> > 1. Add a new file to the securityfs file called sgx/provision that works
> >    as a token for allowing an enclave to have the provisioning privileges.
> > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> >    following data structure:
> >
> >    struct sgx_enclave_set_attribute {
> >            __u64 addr;
> >            __u64 token_fd;
> >    };
> 
> Here's a potential issue:
> 
> For container use, is it reasonable for a container manager to
> bind-mount a file into securityfs?  Or would something in /dev make
> this easier?

I guess that is a valid point given that the securityfs contains the LSM
(e.g. SELinux or AppArmor) policy. So yeah, I think your are right what
you say.

I propose that we create /dev/sgx/enclave to act as the enclave manager
and /dev/sgx/provision for provisioning. Is this sustainable for you?

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Fri, Mar 22, 2019 at 01:29:38PM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 21, 2019 at 09:50:41AM -0700, Andy Lutomirski wrote:
> > On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > In order to provide a mechanism for devilering provisoning rights:
> > >
> > > 1. Add a new file to the securityfs file called sgx/provision that works
> > >    as a token for allowing an enclave to have the provisioning privileges.
> > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > >    following data structure:
> > >
> > >    struct sgx_enclave_set_attribute {
> > >            __u64 addr;
> > >            __u64 token_fd;
> > >    };
> > 
> > Here's a potential issue:
> > 
> > For container use, is it reasonable for a container manager to
> > bind-mount a file into securityfs?  Or would something in /dev make
> > this easier?
> 
> I guess that is a valid point given that the securityfs contains the LSM
> (e.g. SELinux or AppArmor) policy. So yeah, I think your are right what
> you say.
> 
> I propose that we create /dev/sgx/enclave to act as the enclave manager
> and /dev/sgx/provision for provisioning. Is this sustainable for you?

Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
that the container manager can only access specific files inside
securityfs. With this conclusion I still think it is probably the best
place for seurity policy like things even for SGX. It is meant for that
anyway.

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Andy Lutomirski]

On Fri, Mar 22, 2019 at 4:43 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, Mar 22, 2019 at 01:29:38PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 21, 2019 at 09:50:41AM -0700, Andy Lutomirski wrote:
> > > On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > In order to provide a mechanism for devilering provisoning rights:
> > > >
> > > > 1. Add a new file to the securityfs file called sgx/provision that works
> > > >    as a token for allowing an enclave to have the provisioning privileges.
> > > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > > >    following data structure:
> > > >
> > > >    struct sgx_enclave_set_attribute {
> > > >            __u64 addr;
> > > >            __u64 token_fd;
> > > >    };
> > >
> > > Here's a potential issue:
> > >
> > > For container use, is it reasonable for a container manager to
> > > bind-mount a file into securityfs?  Or would something in /dev make
> > > this easier?
> >
> > I guess that is a valid point given that the securityfs contains the LSM
> > (e.g. SELinux or AppArmor) policy. So yeah, I think your are right what
> > you say.
> >
> > I propose that we create /dev/sgx/enclave to act as the enclave manager
> > and /dev/sgx/provision for provisioning. Is this sustainable for you?
>
> Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> that the container manager can only access specific files inside
> securityfs. With this conclusion I still think it is probably the best
> place for seurity policy like things even for SGX. It is meant for that
> anyway.
>

LSM or DAC policy can certainly *restrict* it, but I suspect that most
container runtimes don't mount securityfs at all.  OTOH, the runtime
definitely needs to have a way to pass /dev/sgx/enclave (or whatever
it's called) through, so using another device node will definitely
work.

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Fri, Mar 22, 2019 at 11:20:30AM -0700, Andy Lutomirski wrote:
> On Fri, Mar 22, 2019 at 4:43 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, Mar 22, 2019 at 01:29:38PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 21, 2019 at 09:50:41AM -0700, Andy Lutomirski wrote:
> > > > On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > >
> > > > > In order to provide a mechanism for devilering provisoning rights:
> > > > >
> > > > > 1. Add a new file to the securityfs file called sgx/provision that works
> > > > >    as a token for allowing an enclave to have the provisioning privileges.
> > > > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > > > >    following data structure:
> > > > >
> > > > >    struct sgx_enclave_set_attribute {
> > > > >            __u64 addr;
> > > > >            __u64 token_fd;
> > > > >    };
> > > >
> > > > Here's a potential issue:
> > > >
> > > > For container use, is it reasonable for a container manager to
> > > > bind-mount a file into securityfs?  Or would something in /dev make
> > > > this easier?
> > >
> > > I guess that is a valid point given that the securityfs contains the LSM
> > > (e.g. SELinux or AppArmor) policy. So yeah, I think your are right what
> > > you say.
> > >
> > > I propose that we create /dev/sgx/enclave to act as the enclave manager
> > > and /dev/sgx/provision for provisioning. Is this sustainable for you?
> >
> > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > that the container manager can only access specific files inside
> > securityfs. With this conclusion I still think it is probably the best
> > place for seurity policy like things even for SGX. It is meant for that
> > anyway.
> >
> 
> LSM or DAC policy can certainly *restrict* it, but I suspect that most
> container runtimes don't mount securityfs at all.  OTOH, the runtime
> definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> it's called) through, so using another device node will definitely
> work.

OK, I can cope with this argument. I go with the device names above for
v20.

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Sean Christopherson]

On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 22, 2019 at 11:20:30AM -0700, Andy Lutomirski wrote:
> > On Fri, Mar 22, 2019 at 4:43 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Fri, Mar 22, 2019 at 01:29:38PM +0200, Jarkko Sakkinen wrote:
> > > > On Thu, Mar 21, 2019 at 09:50:41AM -0700, Andy Lutomirski wrote:
> > > > > On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
> > > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > > >
> > > > > > In order to provide a mechanism for devilering provisoning rights:
> > > > > >
> > > > > > 1. Add a new file to the securityfs file called sgx/provision that works
> > > > > >    as a token for allowing an enclave to have the provisioning privileges.
> > > > > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > > > > >    following data structure:
> > > > > >
> > > > > >    struct sgx_enclave_set_attribute {
> > > > > >            __u64 addr;
> > > > > >            __u64 token_fd;
> > > > > >    };
> > > > >
> > > > > Here's a potential issue:
> > > > >
> > > > > For container use, is it reasonable for a container manager to
> > > > > bind-mount a file into securityfs?  Or would something in /dev make
> > > > > this easier?
> > > >
> > > > I guess that is a valid point given that the securityfs contains the LSM
> > > > (e.g. SELinux or AppArmor) policy. So yeah, I think your are right what
> > > > you say.
> > > >
> > > > I propose that we create /dev/sgx/enclave to act as the enclave manager
> > > > and /dev/sgx/provision for provisioning. Is this sustainable for you?
> > >
> > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > that the container manager can only access specific files inside
> > > securityfs. With this conclusion I still think it is probably the best
> > > place for seurity policy like things even for SGX. It is meant for that
> > > anyway.
> > >
> > 
> > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > it's called) through, so using another device node will definitely
> > work.
> 
> OK, I can cope with this argument. I go with the device names above for
> v20.

Regardless of where the file ends up living, I think it should be owned
by the "core" SGX subsystem and not the driver.  At some point in the
past Andy requested that KVM intercept EINIT as needed to prevent
unauthorized access to the PROVISION_KEY (by running a KVM guest),
I.e. userspace may want to access /dev/sgx/provision or whatever even
if the driver is not loaded.  I don't see any reason to defer that change
to the KVM series.

If the ACPI-based autoprobing is removed, then sgx_init() can remove the
file if probing the driver fails, i.e. the kernel won't end up with a
file that userspace can't use for anything.

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > that the container manager can only access specific files inside
> > > securityfs. With this conclusion I still think it is probably the best
> > > place for seurity policy like things even for SGX. It is meant for that
> > > anyway.
> > >
> > 
> > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > it's called) through, so using another device node will definitely
> > work.
> 
> OK, I can cope with this argument. I go with the device names above for
> v20.

In v20 the refactoring would be with corresponding modes:

/dev/sgx 0755
/dev/sgx/enclave 0666
/dev/sgx/provision 0600

The problem that I'm facing is that with devnode callback of struct
device_type I can easily give the defaut mode for any of the files but
not for the /dev/sgx directory itself. How do I get the appropriate
mode for it?

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Andy Lutomirski]

On Fri, Apr 5, 2019 at 3:18 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> > > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > > that the container manager can only access specific files inside
> > > > securityfs. With this conclusion I still think it is probably the best
> > > > place for seurity policy like things even for SGX. It is meant for that
> > > > anyway.
> > > >
> > >
> > > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > > it's called) through, so using another device node will definitely
> > > work.
> >
> > OK, I can cope with this argument. I go with the device names above for
> > v20.
>
> In v20 the refactoring would be with corresponding modes:
>
> /dev/sgx 0755
> /dev/sgx/enclave 0666
> /dev/sgx/provision 0600
>
> The problem that I'm facing is that with devnode callback of struct
> device_type I can easily give the defaut mode for any of the files but
> not for the /dev/sgx directory itself. How do I get the appropriate
> mode for it?
>

Hi Greg-

Do you know this one?

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Fri, Apr 05, 2019 at 06:53:57AM -0700, Andy Lutomirski wrote:
> On Fri, Apr 5, 2019 at 3:18 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> > > > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > > > that the container manager can only access specific files inside
> > > > > securityfs. With this conclusion I still think it is probably the best
> > > > > place for seurity policy like things even for SGX. It is meant for that
> > > > > anyway.
> > > > >
> > > >
> > > > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > > > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > > > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > > > it's called) through, so using another device node will definitely
> > > > work.
> > >
> > > OK, I can cope with this argument. I go with the device names above for
> > > v20.
> >
> > In v20 the refactoring would be with corresponding modes:
> >
> > /dev/sgx 0755
> > /dev/sgx/enclave 0666
> > /dev/sgx/provision 0600
> >
> > The problem that I'm facing is that with devnode callback of struct
> > device_type I can easily give the defaut mode for any of the files but
> > not for the /dev/sgx directory itself. How do I get the appropriate
> > mode for it?
> >
> 
> Hi Greg-
> 
> Do you know this one?

I guess one option is to not do anything with the mode but instead
contribute rules to udev? I'm not too familiar with this but maybe
that is even recommended way?

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Greg KH]

On Fri, Apr 05, 2019 at 05:20:06PM +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 05, 2019 at 06:53:57AM -0700, Andy Lutomirski wrote:
> > On Fri, Apr 5, 2019 at 3:18 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> > > > > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > > > > that the container manager can only access specific files inside
> > > > > > securityfs. With this conclusion I still think it is probably the best
> > > > > > place for seurity policy like things even for SGX. It is meant for that
> > > > > > anyway.
> > > > > >
> > > > >
> > > > > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > > > > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > > > > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > > > > it's called) through, so using another device node will definitely
> > > > > work.
> > > >
> > > > OK, I can cope with this argument. I go with the device names above for
> > > > v20.
> > >
> > > In v20 the refactoring would be with corresponding modes:
> > >
> > > /dev/sgx 0755
> > > /dev/sgx/enclave 0666
> > > /dev/sgx/provision 0600
> > >
> > > The problem that I'm facing is that with devnode callback of struct
> > > device_type I can easily give the defaut mode for any of the files but
> > > not for the /dev/sgx directory itself. How do I get the appropriate
> > > mode for it?
> > >
> > 
> > Hi Greg-
> > 
> > Do you know this one?
> 
> I guess one option is to not do anything with the mode but instead
> contribute rules to udev? I'm not too familiar with this but maybe
> that is even recommended way?

Why do you care about the directory permissions?  They should not
matter, only the permissions on the device nodes themselves, right?

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Jarkko Sakkinen]

On Fri, Apr 05, 2019 at 04:34:46PM +0200, Greg KH wrote:
> On Fri, Apr 05, 2019 at 05:20:06PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Apr 05, 2019 at 06:53:57AM -0700, Andy Lutomirski wrote:
> > > On Fri, Apr 5, 2019 at 3:18 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> > > > > > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > > > > > that the container manager can only access specific files inside
> > > > > > > securityfs. With this conclusion I still think it is probably the best
> > > > > > > place for seurity policy like things even for SGX. It is meant for that
> > > > > > > anyway.
> > > > > > >
> > > > > >
> > > > > > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > > > > > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > > > > > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > > > > > it's called) through, so using another device node will definitely
> > > > > > work.
> > > > >
> > > > > OK, I can cope with this argument. I go with the device names above for
> > > > > v20.
> > > >
> > > > In v20 the refactoring would be with corresponding modes:
> > > >
> > > > /dev/sgx 0755
> > > > /dev/sgx/enclave 0666
> > > > /dev/sgx/provision 0600
> > > >
> > > > The problem that I'm facing is that with devnode callback of struct
> > > > device_type I can easily give the defaut mode for any of the files but
> > > > not for the /dev/sgx directory itself. How do I get the appropriate
> > > > mode for it?
> > > >
> > > 
> > > Hi Greg-
> > > 
> > > Do you know this one?
> > 
> > I guess one option is to not do anything with the mode but instead
> > contribute rules to udev? I'm not too familiar with this but maybe
> > that is even recommended way?
> 
> Why do you care about the directory permissions?  They should not
> matter, only the permissions on the device nodes themselves, right?

OK, based on your response to Andy I don't :-)

Thanks.

/Jarkko

Re: [PATCH v19 17/27] x86/sgx: Add provisioning

[Greg KH]

On Fri, Apr 05, 2019 at 06:53:57AM -0700, Andy Lutomirski wrote:
> On Fri, Apr 5, 2019 at 3:18 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 04:55:03PM +0200, Jarkko Sakkinen wrote:
> > > > > Hmm.. on 2nd thought the LSM policy or even DAC policy  would restrict
> > > > > that the container manager can only access specific files inside
> > > > > securityfs. With this conclusion I still think it is probably the best
> > > > > place for seurity policy like things even for SGX. It is meant for that
> > > > > anyway.
> > > > >
> > > >
> > > > LSM or DAC policy can certainly *restrict* it, but I suspect that most
> > > > container runtimes don't mount securityfs at all.  OTOH, the runtime
> > > > definitely needs to have a way to pass /dev/sgx/enclave (or whatever
> > > > it's called) through, so using another device node will definitely
> > > > work.
> > >
> > > OK, I can cope with this argument. I go with the device names above for
> > > v20.
> >
> > In v20 the refactoring would be with corresponding modes:
> >
> > /dev/sgx 0755
> > /dev/sgx/enclave 0666
> > /dev/sgx/provision 0600
> >
> > The problem that I'm facing is that with devnode callback of struct
> > device_type I can easily give the defaut mode for any of the files but
> > not for the /dev/sgx directory itself. How do I get the appropriate
> > mode for it?
> >
> 
> Hi Greg-
> 
> Do you know this one?

You can't get the mode of the directory, it is always 0755 for devtmpfs,
is that a problem?  If so, write a udev rule to change it :)

thanks,

greg k-h