From: Jan Beulich <jbeulich@suse.com>
To: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
Cc: "Petre Ovidiu PIRCALABU" <ppircalabu@bitdefender.com>,
"Kevin Tian" <kevin.tian@intel.com>,
"Tamas K Lengyel" <tamas@tklengyel.com>, "Wei Liu" <wl@xen.org>,
"Razvan COJOCARU" <rcojocaru@bitdefender.com>,
"George Dunlap" <george.dunlap@eu.citrix.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"Jun Nakajima" <jun.nakajima@intel.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH V4 1/4] x86/mm: Add array_index_nospec to guest provided index values
Date: Wed, 18 Dec 2019 11:06:21 +0100 [thread overview]
Message-ID: <75e8fff3-79fc-1363-a212-fb33dc8776bc@suse.com> (raw)
In-Reply-To: <131f196c-e149-3cec-765b-be7bf36bf19b@bitdefender.com>
On 18.12.2019 10:57, Alexandru Stefan ISAILA wrote:
> On 18.12.2019 10:06, Alexandru Stefan ISAILA wrote:
>> On 17.12.2019 18:50, Jan Beulich wrote:
>>> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -1353,7 +1353,8 @@ void setup_ept_dump(void)
>>>>
>>>> void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>> {
>>>> - struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>>> + struct p2m_domain *p2m =
>>>> + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)];
>>>> struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>> struct ept_data *ept;
>>>>
>>>> @@ -1366,7 +1367,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>> p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0;
>>>> ept = &p2m->ept;
>>>> ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>> - d->arch.altp2m_eptp[i] = ept->eptp;
>>>> + d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
>>>> }
>>>>
>>>> unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -2499,7 +2499,7 @@ static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>>>> struct p2m_domain *p2m;
>>>>
>>>> ASSERT(idx < MAX_ALTP2M);
>>>> - p2m = d->arch.altp2m_p2m[idx];
>>>> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
>>>>
>>>> p2m_lock(p2m);
>>>>
>>>> @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
>>>>
>>>> ASSERT(idx < MAX_ALTP2M);
>>>>
>>>> - p2m = d->arch.altp2m_p2m[idx];
>>>> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
>>>
>>> All of the above have a more or less significant disconnect between
>>> the bounds check and the use as array index. I think it would be
>>> quite helpful if these could live close to one another, so one can
>>> (see further up) easily prove that both specified bounds actually
>>> match up.
>>>
>>
>> Sure, I can move the array use closer together.
>>
>
> Sorry to come back on this but I was looking in the code and I am not
> sure I follow where is the disconnect. If you are talking about
> p2m_init_altp2m_ept() the eptp code will move up in patch 3/4.
My remark was about all four hunks left in context (and then still
possibly extending to other ones). Let's take the last one above:
p2m_activate_altp2m() has two callers, one of which loops over
altp2m-s (and hence doesn't need the guard). The other one is
p2m_init_altp2m_by_id() which does the range check I'm talking
about (ASSERT() doesn't count), and which therefore is the place
to use array_index_nospec(). Once you look there you'll notice
that the function also has an array access itself which you've
left untouched.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-12-18 10:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-17 15:12 [Xen-devel] [PATCH V4 1/4] x86/mm: Add array_index_nospec to guest provided index values Alexandru Stefan ISAILA
2019-12-17 15:12 ` [Xen-devel] [PATCH V4 2/4] x86/altp2m: Add hypercall to set a range of sve bits Alexandru Stefan ISAILA
2019-12-17 17:00 ` Jan Beulich
2019-12-18 8:13 ` Alexandru Stefan ISAILA
2019-12-18 8:45 ` Alexandru Stefan ISAILA
2019-12-18 10:18 ` Jan Beulich
2019-12-18 10:32 ` Alexandru Stefan ISAILA
2019-12-17 15:12 ` [Xen-devel] [PATCH V4 3/4] x86/mm: Pull out the p2m specifics from p2m_init_altp2m_ept Alexandru Stefan ISAILA
2019-12-18 10:36 ` Jan Beulich
2019-12-17 15:12 ` [Xen-devel] [PATCH V4 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view Alexandru Stefan ISAILA
2019-12-18 10:45 ` Jan Beulich
2019-12-18 11:53 ` Alexandru Stefan ISAILA
2019-12-17 15:21 ` [Xen-devel] [PATCH V4 1/4] x86/mm: Add array_index_nospec to guest provided index values Tamas K Lengyel
2019-12-17 16:50 ` Jan Beulich
2019-12-18 8:06 ` Alexandru Stefan ISAILA
2019-12-18 9:57 ` Alexandru Stefan ISAILA
2019-12-18 10:06 ` Jan Beulich [this message]
2019-12-18 10:26 ` Alexandru Stefan ISAILA
2019-12-18 9:59 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=75e8fff3-79fc-1363-a212-fb33dc8776bc@suse.com \
--to=jbeulich@suse.com \
--cc=aisaila@bitdefender.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@eu.citrix.com \
--cc=jun.nakajima@intel.com \
--cc=kevin.tian@intel.com \
--cc=ppircalabu@bitdefender.com \
--cc=rcojocaru@bitdefender.com \
--cc=roger.pau@citrix.com \
--cc=tamas@tklengyel.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.