| Message ID | 20190317211456.13927-18-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX1 support | expand |
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, ¶ms); > > 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 >
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, ¶ms); > > > > 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
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, ¶ms); > > > > 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
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
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, ¶ms); > > 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 >
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?
> 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
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
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
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
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
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.
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
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.
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
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?
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
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
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?
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
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);
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, ¶ms); 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(+)