| Message ID | 20190819152544.7296-5-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Improve permission handing | expand |
On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote: > The validation of TCS permissions was missing from > sgx_validate_secinfo(). This patch adds the validation. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > index 99b1b9776c3a..2415dcb20a6e 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo) > if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) > return -EINVAL; > > + /* CPU will silently overwrite the permissions as zero, which means > + * that we need to validate it ourselves. > + */ > + if (page_type == SGX_SECINFO_TCS && perm) > + return -EINVAL; > + > if (secinfo->flags & SGX_SECINFO_RESERVED_MASK) > return -EINVAL; > > -- > 2.20.1 > OK, just found out that this patch did not end up to my test image and causes a regression. I think this should be fixed in sgx_encl_may_map() by having the following special case for TCS (in addition to the change in this patch of course): 1. Check if we encounter a TCS page. 2. If yes, we evaluate RW for the VM flags. /Jarkko
On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote: > The validation of TCS permissions was missing from > sgx_validate_secinfo(). This patch adds the validation. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > index 99b1b9776c3a..2415dcb20a6e 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo) > if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) > return -EINVAL; > > + /* CPU will silently overwrite the permissions as zero, which means Comment style again. I looked it up just to double check :) Documentation/process/coding-style.rst says: When commenting the kernel API functions, please use the kernel-doc format. See the files at :ref:`Documentation/doc-guide/ <doc_guide>` and ``scripts/kernel-doc`` for details. The preferred style for long (multi-line) comments is: .. code-block:: c /* * This is the preferred style for multi-line * comments in the Linux kernel source code. * Please use it consistently. * * Description: A column of asterisks on the left side, * with beginning and ending almost-blank lines. */ For files in net/ and drivers/net/ the preferred style for long (multi-line) comments is a little different. .. code-block:: c /* The preferred comment style for files in net/ and drivers/net * looks like this. * * It is nearly the same as the generally preferred comment style, * but there is no initial almost-blank line. */ > + * that we need to validate it ourselves. > + */ > + if (page_type == SGX_SECINFO_TCS && perm) > + return -EINVAL; Why are we validating the TCS protection bits? Hardware ignores them, so why do we care? sgx_ioc_enclave_add_page() sets the internal protection bits so there's no danger of putting the wrong thing in the page tables. > + > if (secinfo->flags & SGX_SECINFO_RESERVED_MASK) > return -EINVAL; > > -- > 2.20.1 >
> From: linux-sgx-owner@vger.kernel.org <linux-sgx-owner@vger.kernel.org> > On Behalf Of Jarkko Sakkinen > Sent: Wednesday, August 21, 2019 21:46 > To: linux-sgx@vger.kernel.org > Cc: Christopherson, Sean J <sean.j.christopherson@intel.com> > Subject: Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in > sgx_validate_secinfo() > > On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote: > > The validation of TCS permissions was missing from > > sgx_validate_secinfo(). This patch adds the validation. > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > > b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > > index 99b1b9776c3a..2415dcb20a6e 100644 > > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > > @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo > *secinfo) > > if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) > > return -EINVAL; > > > > + /* CPU will silently overwrite the permissions as zero, which means > > + * that we need to validate it ourselves. > > + */ > > + if (page_type == SGX_SECINFO_TCS && perm) > > + return -EINVAL; > > + > > if (secinfo->flags & SGX_SECINFO_RESERVED_MASK) > > return -EINVAL; > > > > -- > > 2.20.1 > > > > OK, just found out that this patch did not end up to my test image and > causes a regression. > > I think this should be fixed in sgx_encl_may_map() by having the > following special case for TCS (in addition to the change in this > patch of course): > > 1. Check if we encounter a TCS page. > 2. If yes, we evaluate RW for the VM flags. > Also replying to Sean. Sean is right that never mind the value in secsinfo->flags, HW will reset RWX For TCS pages. So basically you may not enforce and and could not check those but... The signature depends On those flags, so if you put a non-zero flag value, eadd will pass but if you compute the signature according to this non zero value then you will have a delta between ur signature and HW's signature: einit will fail. So this is tricky and more a usability issue. I vote for checking the flag is zeroed. --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Thu, Aug 22, 2019 at 04:33:35AM -0700, Ayoun, Serge wrote: > Sean is right that never mind the value in secsinfo->flags, HW will reset RWX > For TCS pages. So basically you may not enforce and and could not check > those but... The signature depends On those flags, so if you put a non-zero > flag value, eadd will pass but if you compute the signature according to this > non zero value then you will have a delta between ur signature and HW's > signature: einit will fail. So this is tricky and more a usability issue. I > vote for checking the flag is zeroed. Ugh, didn't think about that behavior. That's obnoxious. Adding the check makes sense in that case.
On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote: > Why are we validating the TCS protection bits? Hardware ignores them, so > why do we care? sgx_ioc_enclave_add_page() sets the internal protection > bits so there's no danger of putting the wrong thing in the page tables. I think that in this commit I got it wrong but I think this is awkward: /* * TCS pages must always RW set for CPU access while the SECINFO * permissions are *always* zero - the CPU ignores the user provided * values and silently overwrites with zero permissions. */ if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) prot |= PROT_READ | PROT_WRITE; In my opinion the right thing to do would be check that SECINFO has *at minimum* RW and return -EINVAL if not. I don't like the SGX silently adjusting permissions like this. /Jarkko
On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote: > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote: > > Why are we validating the TCS protection bits? Hardware ignores them, so > > why do we care? sgx_ioc_enclave_add_page() sets the internal protection > > bits so there's no danger of putting the wrong thing in the page tables. > > I think that in this commit I got it wrong but I think this is awkward: > > /* > * TCS pages must always RW set for CPU access while the SECINFO > * permissions are *always* zero - the CPU ignores the user provided > * values and silently overwrites with zero permissions. > */ > if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) > prot |= PROT_READ | PROT_WRITE; > > In my opinion the right thing to do would be check that SECINFO has *at > minimum* RW and return -EINVAL if not. Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after* it overwrites the flags for TCS pages. I.e. requiring RW for the TCS would result in every enclave failing EINIT due to an invalid measurement. It'd be fairly easy to verify this if we want to triple check that that is indeed hardware behavior.
On Thu, 2019-08-22 at 19:31 +0300, Jarkko Sakkinen wrote: > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote: > > Why are we validating the TCS protection bits? Hardware ignores them, so > > why do we care? sgx_ioc_enclave_add_page() sets the internal protection > > bits so there's no danger of putting the wrong thing in the page tables. > > I think that in this commit I got it wrong but I think this is awkward: > > /* > * TCS pages must always RW set for CPU access while the SECINFO > * permissions are *always* zero - the CPU ignores the user provided > * values and silently overwrites with zero permissions. > */ > if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) > prot |= PROT_READ | PROT_WRITE; > > In my opinion the right thing to do would be check that SECINFO has *at > minimum* RW and return -EINVAL if not. > > I don't like the SGX silently adjusting permissions like this. For me any sane solution goes where we don't have that kind of tweaking probably my "minimum RW" is more sane than my earlier proposal. /Jarkko
On Thu, 2019-08-22 at 11:33 +0000, Ayoun, Serge wrote: > Also replying to Sean. > Sean is right that never mind the value in secsinfo->flags, HW will reset RWX > For TCS pages. > So basically you may not enforce and and could not check those but... The signature depends > On those flags, so if you put a non-zero flag value, eadd will pass but if you > compute the signature according to this non zero value then you will have > a delta between ur signature and HW's signature: einit will fail. > So this is tricky and more a usability issue. > I vote for checking the flag is zeroed. As I responded to Sean that as long as the ioctl does not adjust prot bits I'm cool with any sane solution. What do you think of requiring at minimum RW? Doing that kind of adjusting is just doing fixup's for corrupted data from the user space. /Jarkko
On Thu, 2019-08-22 at 19:46 +0300, Jarkko Sakkinen wrote: > On Thu, 2019-08-22 at 11:33 +0000, Ayoun, Serge wrote: > > Also replying to Sean. > > Sean is right that never mind the value in secsinfo->flags, HW will reset RWX > > For TCS pages. > > So basically you may not enforce and and could not check those but... The signature depends > > On those flags, so if you put a non-zero flag value, eadd will pass but if you > > compute the signature according to this non zero value then you will have > > a delta between ur signature and HW's signature: einit will fail. > > So this is tricky and more a usability issue. > > I vote for checking the flag is zeroed. > > As I responded to Sean that as long as the ioctl does not adjust > prot bits I'm cool with any sane solution. What do you think of > requiring at minimum RW? > > Doing that kind of adjusting is just doing fixup's for corrupted > data from the user space. Kind of missed your comment about EINIT in rush! A valid point and good catch. I still think my 2nd proposal would be more appropriate than this patch. Signatures will work and we don't need special cases anywhere. /Jarkko
On Thu, 2019-08-22 at 09:34 -0700, Sean Christopherson wrote: > On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote: > > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote: > > > Why are we validating the TCS protection bits? Hardware ignores them, so > > > why do we care? sgx_ioc_enclave_add_page() sets the internal protection > > > bits so there's no danger of putting the wrong thing in the page tables. > > > > I think that in this commit I got it wrong but I think this is awkward: > > > > /* > > * TCS pages must always RW set for CPU access while the SECINFO > > * permissions are *always* zero - the CPU ignores the user provided > > * values and silently overwrites with zero permissions. > > */ > > if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) > > prot |= PROT_READ | PROT_WRITE; > > > > In my opinion the right thing to do would be check that SECINFO has *at > > minimum* RW and return -EINVAL if not. > > Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after* > it overwrites the flags for TCS pages. I.e. requiring RW for the TCS > would result in every enclave failing EINIT due to an invalid measurement. > It'd be fairly easy to verify this if we want to triple check that that is > indeed hardware behavior. This is from the signing tool that I wrote back in 2016 used in the selftest: struct mreadd { uint64_t tag; uint64_t offset; uint64_t flags; /* SECINFO flags */ uint8_t reserved[40]; } __attribute__((__packed__)); static bool mrenclave_eadd(EVP_MD_CTX *ctx, uint64_t offset, uint64_t flags) { struct mreadd mreadd; memset(&mreadd, 0, sizeof(mreadd)); mreadd.tag = MREADD; mreadd.offset = offset; mreadd.flags = flags; return mrenclave_update(ctx, &mreadd); } If MRENCLAVE was updated after the overwrite, this would not work. The least confusing semantics would be to require RW, no more or less. /Jarkko
On Fri, 2019-08-23 at 03:39 +0300, Jarkko Sakkinen wrote: > On Thu, 2019-08-22 at 09:34 -0700, Sean Christopherson wrote: > > On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote: > > > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote: > > > > Why are we validating the TCS protection bits? Hardware ignores them, so > > > > why do we care? sgx_ioc_enclave_add_page() sets the internal protection > > > > bits so there's no danger of putting the wrong thing in the page tables. > > > > > > I think that in this commit I got it wrong but I think this is awkward: > > > > > > /* > > > * TCS pages must always RW set for CPU access while the SECINFO > > > * permissions are *always* zero - the CPU ignores the user provided > > > * values and silently overwrites with zero permissions. > > > */ > > > if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) > > > prot |= PROT_READ | PROT_WRITE; > > > > > > In my opinion the right thing to do would be check that SECINFO has *at > > > minimum* RW and return -EINVAL if not. > > > > Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after* > > it overwrites the flags for TCS pages. I.e. requiring RW for the TCS > > would result in every enclave failing EINIT due to an invalid measurement. > > It'd be fairly easy to verify this if we want to triple check that that is > > indeed hardware behavior. > > This is from the signing tool that I wrote back in 2016 used in the > selftest: > > struct mreadd { > uint64_t tag; > uint64_t offset; > uint64_t flags; /* SECINFO flags */ > uint8_t reserved[40]; > } __attribute__((__packed__)); > > static bool mrenclave_eadd(EVP_MD_CTX *ctx, uint64_t offset, uint64_t flags) > { > struct mreadd mreadd; > > memset(&mreadd, 0, sizeof(mreadd)); > mreadd.tag = MREADD; > mreadd.offset = offset; > mreadd.flags = flags; > > return mrenclave_update(ctx, &mreadd); > } > > If MRENCLAVE was updated after the overwrite, this would not work. > > The least confusing semantics would be to require RW, no more or less. OK, it is how Serge said. This can we verified from the SDM easily (SCRATCH_SECINFO gets zeros is extended after that). And also from my signing tool :-) for (offset = 0; offset < sb.st_size; offset += 0x1000) { if (!offset) flags = SGX_SECINFO_TCS; else flags = SGX_SECINFO_REG | SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X; OK, so this looks like that my patch does exactly the right thing, right? /Jarkko
On Fri, Aug 23, 2019 at 03:57:36AM +0300, Jarkko Sakkinen wrote: > On Fri, 2019-08-23 at 03:39 +0300, Jarkko Sakkinen wrote: > > On Thu, 2019-08-22 at 09:34 -0700, Sean Christopherson wrote: > > > On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote: > > > > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote: > > > > > Why are we validating the TCS protection bits? Hardware ignores them, so > > > > > why do we care? sgx_ioc_enclave_add_page() sets the internal protection > > > > > bits so there's no danger of putting the wrong thing in the page tables. > > > > > > > > I think that in this commit I got it wrong but I think this is awkward: > > > > > > > > /* > > > > * TCS pages must always RW set for CPU access while the SECINFO > > > > * permissions are *always* zero - the CPU ignores the user provided > > > > * values and silently overwrites with zero permissions. > > > > */ > > > > if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) > > > > prot |= PROT_READ | PROT_WRITE; > > > > > > > > In my opinion the right thing to do would be check that SECINFO has *at > > > > minimum* RW and return -EINVAL if not. > > > > > > Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after* > > > it overwrites the flags for TCS pages. I.e. requiring RW for the TCS > > > would result in every enclave failing EINIT due to an invalid measurement. > > > It'd be fairly easy to verify this if we want to triple check that that is > > > indeed hardware behavior. > > > > This is from the signing tool that I wrote back in 2016 used in the > > selftest: > > > > struct mreadd { > > uint64_t tag; > > uint64_t offset; > > uint64_t flags; /* SECINFO flags */ > > uint8_t reserved[40]; > > } __attribute__((__packed__)); > > > > static bool mrenclave_eadd(EVP_MD_CTX *ctx, uint64_t offset, uint64_t flags) > > { > > struct mreadd mreadd; > > > > memset(&mreadd, 0, sizeof(mreadd)); > > mreadd.tag = MREADD; > > mreadd.offset = offset; > > mreadd.flags = flags; > > > > return mrenclave_update(ctx, &mreadd); > > } > > > > If MRENCLAVE was updated after the overwrite, this would not work. > > > > The least confusing semantics would be to require RW, no more or less. > > OK, it is how Serge said. > > This can we verified from the SDM easily (SCRATCH_SECINFO gets zeros > is extended after that). > > And also from my signing tool :-) > > for (offset = 0; offset < sb.st_size; offset += 0x1000) { > if (!offset) > flags = SGX_SECINFO_TCS; > else > flags = SGX_SECINFO_REG | SGX_SECINFO_R | > SGX_SECINFO_W | SGX_SECINFO_X; > > OK, so this looks like that my patch does exactly the right thing, > right? That's my understanding as well. Definitely worthy of a comment explaining all of the above.
On Thu, 2019-08-22 at 19:05 -0700, Sean Christopherson wrote: > > This can we verified from the SDM easily (SCRATCH_SECINFO gets zeros > > is extended after that). > > > > And also from my signing tool :-) > > > > for (offset = 0; offset < sb.st_size; offset += 0x1000) { > > if (!offset) > > flags = SGX_SECINFO_TCS; > > else > > flags = SGX_SECINFO_REG | SGX_SECINFO_R | > > SGX_SECINFO_W | SGX_SECINFO_X; > > > > OK, so this looks like that my patch does exactly the right thing, > > right? > > That's my understanding as well. Definitely worthy of a comment > explaining all of the above. Now that I looked at my own code I even remember going through this same thought process three years ago when I wrote that :-) Oh well. So should I apply my zero check patch? /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index 99b1b9776c3a..2415dcb20a6e 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo) if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) return -EINVAL; + /* CPU will silently overwrite the permissions as zero, which means + * that we need to validate it ourselves. + */ + if (page_type == SGX_SECINFO_TCS && perm) + return -EINVAL; + if (secinfo->flags & SGX_SECINFO_RESERVED_MASK) return -EINVAL;
The validation of TCS permissions was missing from sgx_validate_secinfo(). This patch adds the validation. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)