| 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
| ||
|
Message-ID: <SA1PR21MB1335736123C2BCBBFD7460C3BF46A@SA1PR21MB1335.namprd21.prod.outlook.com>
Date: Thu, 25 May 2023 02:06:20 +0000
From: Dexuan Cui <decui@...rosoft.com>
To: Dave Hansen <dave.hansen@...el.com>,
"ak@...ux.intel.com" <ak@...ux.intel.com>,
"arnd@...db.de" <arnd@...db.de>, "bp@...en8.de" <bp@...en8.de>,
"brijesh.singh@....com" <brijesh.singh@....com>,
"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
"hpa@...or.com" <hpa@...or.com>,
"jane.chu@...cle.com" <jane.chu@...cle.com>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
KY Srinivasan <kys@...rosoft.com>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"luto@...nel.org" <luto@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"sathyanarayanan.kuppuswamy@...ux.intel.com"
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
"seanjc@...gle.com" <seanjc@...gle.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"tony.luck@...el.com" <tony.luck@...el.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
"x86@...nel.org" <x86@...nel.org>,
"Michael Kelley (LINUX)" <mikelley@...rosoft.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Tianyu Lan <Tianyu.Lan@...rosoft.com>
Subject: RE: [PATCH v6 1/6] x86/tdx: Retry TDVMCALL_MAP_GPA() when needed
> From: Dave Hansen <dave.hansen@...el.com>
> Sent: Tuesday, May 23, 2023 2:13 PM
> ...
> On 5/4/23 15:53, Dexuan Cui wrote:
> > - if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
> > + while (1) {
> > + memset(&args, 0, sizeof(args));
> > + args.r10 = TDX_HYPERCALL_STANDARD;
> > + args.r11 = TDVMCALL_MAP_GPA;
> > + args.r12 = start;
> > + args.r13 = end - start;
> > +
> > + ret = __tdx_hypercall_ret(&args);
> > + if (ret != TDVMCALL_STATUS_RETRY)
> > + break;
> > + /*
> > + * The guest must retry the operation for the pages in the
> > + * region starting at the GPA specified in R11. Make sure R11
> > + * contains a sane value.
> > + */
> > + map_fail_paddr = args.r11;
> > + if (map_fail_paddr < start || map_fail_paddr >= end)
> > + return false;
>
> This should probably also say: "r11" comes from the untrusted VMM.
> Sanity check it.
Thanks! I'll use the wording you recommended in the next version.
> Should this *really* be "map_fail_paddr >= end"? Or is
> "map_fail_paddr > end" sufficient. In other words, is it really
> worth failing this if a VMM said to retry a 0-byte region at the end?
According to the GHCI spec, R13 "must be a multiple of 4KB". My
understanding is that R13 should not be 0, and a hypervisor is not
supposed to tell the guest to retry a 0-byte region at the end.
IMHO it should be a hypervisor bug if a hypervisor returns
TDVMCALL_STATUS_RETRY and the returned 'map_fail_paddr' equals
'end' (Note: the valid page range is [start, end - 1]).
Hyper-V returns "invalid parameter" if the length (i.e. args.r13) is 0,
so "retry a 0-byte region at the end" would fail anyway. I guess
other hypervisors may also return an error if the length is 0.
So I'd like to keep the comparison as-is.
> > + if (map_fail_paddr == start) {
> > + retry_cnt++;
> > + if (retry_cnt > max_retry_cnt)
>
> I think we can spare two bytes in a few spots to make these 'count'
> instead of 'cnt'.
Ok, I'll rename the variable 'max_retry_cnt' to 'max_retries_per_page',
and 'retry_cnt' to 'retry_count'.
> > + return false;
> > + } else {
> > + retry_cnt = 0;
> > + start = map_fail_paddr;
> > + }
> > + }
>
> this fails the "normal operation should be at the lowest indentation"
> rule. How about this:
>
> while (retry_count < max_retries) {
> ...
>
> /* "Consume" a retry without forward progress: */
> if (map_fail_paddr == start) {
> retry_count++;
> continue;
> }
>
> start = map_fail_paddr;
> retry_count = 0;
> }
>
> // plus maybe a wee bit different 'ret' processing
>
>
> 'max_retries' also ends up being a misnomer. You can have as many
> retries as there are pages plus 'max_retries'. It's really "maximum
> allowed consecutive failures". Maybe it should be "max_retries_per_page".
Thanks, I'll raname 'max_retries" to 'max_retries_per_page'.
I'll use the beow in the next version.
I added "const" to "int max_retries_per_page".
Please let me know if I missed anything.
+static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
+{
+ const int max_retries_per_page = 3;
+ struct tdx_hypercall_args args;
+ u64 map_fail_paddr, ret;
+ int retry_count = 0;
+
+ if (!enc) {
+ /* Set the shared (decrypted) bits: */
+ start |= cc_mkdec(0);
+ end |= cc_mkdec(0);
+ }
+
+ while (retry_count < max_retries_per_page) {
+ memset(&args, 0, sizeof(args));
+ args.r10 = TDX_HYPERCALL_STANDARD;
+ args.r11 = TDVMCALL_MAP_GPA;
+ args.r12 = start;
+ args.r13 = end - start;
+
+ ret = __tdx_hypercall_ret(&args);
+ if (ret != TDVMCALL_STATUS_RETRY)
+ return !ret;
+ /*
+ * The guest must retry the operation for the pages in the
+ * region starting at the GPA specified in R11. R11 comes
+ * from the untrusted VMM. Sanity check it.
+ */
+ map_fail_paddr = args.r11;
+ if (map_fail_paddr < start || map_fail_paddr >= end)
+ return false;
+
+ /* "Consume" a retry without forward progress */
+ if (map_fail_paddr == start) {
+ retry_count++;
+ continue;
+ }
+
+ start = map_fail_paddr;
+ retry_count = 0;
+ }
+
+ return false;
+}
Powered by blists - more mailing lists