linux-kernel - Re: [PATCH V3 14/30] x86/sgx: Support restricting of enclave page permissions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2bf954521a3cfc7c677887b1eef772cfcc9e3157.camel@kernel.org>
Date:   Wed, 06 Apr 2022 10:30:28 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Reinette Chatre <reinette.chatre@...el.com>,
        dave.hansen@...ux.intel.com, tglx@...utronix.de, bp@...en8.de,
        luto@...nel.org, mingo@...hat.com, linux-sgx@...r.kernel.org,
        x86@...nel.org
Cc:     seanjc@...gle.com, kai.huang@...el.com, cathy.zhang@...el.com,
        cedric.xing@...el.com, haitao.huang@...el.com,
        mark.shanahan@...el.com, hpa@...or.com,
        linux-kernel@...r.kernel.org, nathaniel@...fian.com
Subject: Re: [PATCH V3 14/30] x86/sgx: Support restricting of enclave page
 permissions

On Tue, 2022-04-05 at 11:59 -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 4/5/2022 11:39 AM, Jarkko Sakkinen wrote:
> > On Tue, 2022-04-05 at 09:49 -0700, Reinette Chatre wrote:
> > > Hi Jarkko,
> > > 
> > > On 4/5/2022 7:52 AM, Jarkko Sakkinen wrote:
> > > > n Tue, 2022-04-05 at 17:27 +0300, Jarkko Sakkinen wrote:
> > > > > According to SDM having page type as regular is fine for EMODPR,
> > > > > i.e. that's why I did not care about having it in SECINFO.
> > > > > 
> > > > > Given that the opcode itself contains validation, I wonder
> > > > > why this needs to be done:
> > > > > 
> > > > > if (secinfo.flags & ~SGX_SECINFO_PERMISSION_MASK)
> > > > >                 return -EINVAL;
> > > > > 
> > > > > if (memchr_inv(secinfo.reserved, 0, sizeof(secinfo.reserved)))
> > > > >                 return -EINVAL;
> > > > > 
> > > > > perm = secinfo.flags & SGX_SECINFO_PERMISSION_MASK;
> > > > > 
> > > > > I.e. why duplicate validation and why does it have different
> > > > > invariant than the opcode?
> > > > 
> > > > Right it is done to prevent exceptions and also pseudo-code
> > > > has this validation:
> > > > 
> > > > IF (EPCM(DS:RCX).PT is not PT_REG) THEN #PF(DS:RCX); FI; 
> > > 
> > > The current type of the page is validated - not the page type
> > > provided in the parameters of the command.
> > > 
> > > > 
> > > > This is clearly wrong:
> > > 
> > > Could you please elaborate what is wrong? The hardware only checks
> > > the permission bits and that is what is provided.
> > 
> > I think it's for most a bit confusing that it takes a special Linux
> > defined SECINFO instead of what you read from spec. 
> > 
> > > 
> > > > 
> > > > /*
> > > >  * Return valid permission fields from a secinfo structure provided by
> > > >  * user space. The secinfo structure is required to only have bits in
> > > >  * the permission fields set.
> > > >  */
> > > > static int sgx_perm_from_user_secinfo(void __user *_secinfo, u64 *secinfo_perm)
> > > > 
> > > > It means that the API requires a malformed data as input.
> > > 
> > > It is not clear to me how this is malformed. The API requires that only
> > > the permission bits are set in the secinfo, only the permission bits in secinfo
> > > is provided to the hardware, and the hardware only checks the permission bits.
> > > 
> > > > 
> > > > Maybe it would be better idea then to replace secinfo with just the
> > > > permission field?
> > > 
> > > That is what I implemented in V1 [1], but was asked to change to secinfo. I could
> > > go back to that if you prefer.
> > 
> > Yeah, if I was the one saying that, I was clearly wrong. But also
> > perspective is now very different after using a lot of these
> > features.
> 
> No problem, I understand.
> 
> I plan to replace the current "secinfo" field in struct sgx_enclave_restrict_permissions
> with a new "permissions" field that contain only the permissions. Please let
> me know if you have concerns with this (I also discuss this more in reply to
> your other message related to the page type change ioctl()).

I'm cool with it but if it is named as "permissions", then 
it is already software-defined entity, i.e. meaning just that
have this check in place in the ioctl:

if (addp->permissions & !(PROT_READ | PROT_WRITE | PROT_EXEC))
	return -EINVAL;

BR, Jarkko