Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <0f44fba956288bcad69e076f84118bc50f8e5d2f.camel@kernel.org>
Subject: Re: [PATCH V3 14/30] x86/sgx: Support restricting of enclave page
 permissions
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Reinette Chatre <reinette.chatre@intel.com>,
        dave.hansen@linux.intel.com, tglx@linutronix.de, bp@alien8.de,
        luto@kernel.org, mingo@redhat.com, linux-sgx@vger.kernel.org,
        x86@kernel.org
Cc:     seanjc@google.com, kai.huang@intel.com, cathy.zhang@intel.com,
        cedric.xing@intel.com, haitao.huang@intel.com,
        mark.shanahan@intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org, nathaniel@profian.com
Date:   Tue, 05 Apr 2022 21:39:01 +0300
In-Reply-To: <59910ad4-a898-4eb2-5e2b-856c686b53fb@intel.com>
References: <cover.1648847675.git.reinette.chatre@intel.com>
         <8ed9ee98ca26c9eefde0fd49062bca6e7b9efe80.1648847675.git.reinette.chatre@intel.com>
         <c9071aa19076c7c618294f0c9cb830a8be96ae09.camel@kernel.org>
         <26ab773de8842d03b40caf8645ca86884b195901.camel@kernel.org>
         <91a02d50b2cba34dfb058fd864ba20ef1e6a5554.camel@kernel.org>
         <10303ca73ea02a300636580e87446766374f66cb.camel@kernel.org>
         <6e0feeadc562b9e3f0a524040469d4f5c3484824.camel@kernel.org>
         <ca08465b6fa8af4121592c6381023fda5e0ade70.camel@kernel.org>
         <59910ad4-a898-4eb2-5e2b-856c686b53fb@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.42.4 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2022-04-05 at 09:49 -0700, Reinette Chatre wrote:
> Hi Jarkko,
>=20
> 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.
> > >=20
> > > Given that the opcode itself contains validation, I wonder
> > > why this needs to be done:
> > >=20
> > > if (secinfo.flags & ~SGX_SECINFO_PERMISSION_MASK)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return -EINVAL;
> > >=20
> > > if (memchr_inv(secinfo.reserved, 0, sizeof(secinfo.reserved)))
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return -EINVAL;
> > >=20
> > > perm =3D secinfo.flags & SGX_SECINFO_PERMISSION_MASK;
> > >=20
> > > I.e. why duplicate validation and why does it have different
> > > invariant than the opcode?
> >=20
> > Right it is done to prevent exceptions and also pseudo-code
> > has this validation:
> >=20
> > IF (EPCM(DS:RCX).PT is not PT_REG) THEN #PF(DS:RCX); FI;=20
>=20
> The current type of the page is validated - not the page type
> provided in the parameters of the command.
>=20
> >=20
> > This is clearly wrong:
>=20
> 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.=20

>=20
> >=20
> > /*
> > =C2=A0* Return valid permission fields from a secinfo structure provide=
d by
> > =C2=A0* user space. The secinfo structure is required to only have bits=
 in
> > =C2=A0* the permission fields set.
> > =C2=A0*/
> > static int sgx_perm_from_user_secinfo(void __user *_secinfo, u64 *secin=
fo_perm)
> >=20
> > It means that the API requires a malformed data as input.
>=20
> 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 s=
ecinfo
> is provided to the hardware, and the hardware only checks the permission =
bits.
>=20
> >=20
> > Maybe it would be better idea then to replace secinfo with just the
> > permission field?
>=20
> 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.

Alternatively you could have a single "mod" ioctl given the disjoint
nature how the parameters go to SECINFO.


> Reinette
>=20
> [1] https://lore.kernel.org/linux-sgx/44fe170cfd855760857660b9f56cae8c474=
7cc15.1638381245.git.reinette.chatre@intel.com/

BR, Jarkko