Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
> I will absolutely nack any interface where if the caller says, > "Please > remove read permission", the hypervisor says, "OK!" but then allows > read > permission anyway -- particularly in one which is allegedly designed > for > security tools. > > If it's not practical / more work than it's worth doing at the moment > to > implement p2m_access_n on NPT, then you should return an error when > it's > requested. > > The same really should be true for write-only permission as well -- > if > it's not possible to allow writes but not reads, then you should > return > an error when such permissions are requested. I will limit the supported access rights and return error for read/write only and _n. Regards, Alex ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On 07/19/2018 09:18 AM, Isaila Alexandru wrote:
> On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila >> om> wrote:
>>>
>>> From: Isaila Alexandru
>>>
>>> This patch adds access rights for the NPT pages. The access rights
>>> are
>>> saved in a radix tree with the root saved in p2m_domain. The rights
>>> are manipulated through p2m_set_access()
>>> and p2m_get_access() functions.
>>> The patch follows the ept logic.
>> This description needs to be much more complete. Something like
>> this:
>>
>> ---
>> This patch adds access control for NPT mode.
>>
>> There aren’t enough extra bits to store the access rights in the NPT
>> p2m table, so we add a radix tree to store the rights. For
>> efficiency, remove entries which match the default permissions rather
>> than continuing to store them.
>>
>> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking
>> an access, and removing / adding RW or NX flags as appropriate.
>> ---
>>
> I will update the patch description.
>>>
>>>
>>> Note: It was tested with xen-access write
>>>
>>> Signed-off-by: Alexandru Isaila
>>
>>>
>>>
>>> ---
>>> Changes since V2:
>>> - Delete blak line
>>> - Add return if p2m_access_rwx = a
>>> - Delete the comment from p2m_pt_get_entry()
>>> - Moved radix_tree_init() to arch_monitor_init_domain().
>>> ---
>>> xen/arch/x86/mm/mem_access.c | 3 ++
>>> xen/arch/x86/mm/p2m-pt.c | 109
>>> ++-
>>> xen/arch/x86/mm/p2m.c| 6 +++
>>> xen/arch/x86/monitor.c | 13 +
>>> xen/include/asm-x86/mem_access.h | 2 +-
>>> xen/include/asm-x86/p2m.h| 6 +++
>>> 6 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c
>>> b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..d78c82c 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa,
>>> unsigned long gla,
>>> {
>>> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>>> req->u.mem_access.gla = gla;
>>> +}
>>>
>>> +if ( npfec.gla_valid || cpu_has_svm )
>>> +{
>> I can’t immediately tell what this is about, which means it needs a
>> comment.
>>
>> It may also be (depending on why this is here) that “cpu_has_svm”
>> makes this more fragile — if this is really about having a radix
>> tree, for instance, then you should probably check for a radix tree.
>
> This is about the different npfec on SVN. The gla in never valid so the
> fault flag will not be set.
Right -- then really this check was always a VMX-ism, and should really
have been:
/* VMX can only tell the fault type if gla_valid is set */
if ( !(cpu_has_vmx && !npfec.gla_valid) ) {
...
But in any case, if cpu_has_vmx && !npfec.gla_valid, then npfec.kind
should be npfec_kind_unknown (==0); so is there any reason not to read
npfec.kind here unconditionally? i.e., like below?
if ( npfec.gla_valid )
{
req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
req->u.mem_access.gla = gla;
}
if ( npfec.kind == npfec_kind_with_gla )
req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
else if ( npfec.kind == npfec_kind_in_gpt )
req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On 07/19/2018 02:08 PM, Isaila Alexandru wrote:
> On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
>>>
>
> On 19.07.18 at 10:43, wrote:
>>> On 07/19/2018 11:30 AM, Jan Beulich wrote:
>
>>
>>>
>>> On 19.07.18 at 10:18, wrote:
> On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila >> fender.c
>>> +break;
>>> +case p2m_access_x:
>>> +flags &= ~_PAGE_RW;
>>> +break;
>>> +case p2m_access_rwx:
>>> +default:
>>> +break;
>>> }
>> I think you want another blank line here too.
>>
>> Also, this doesn’t seem to capture the ‘r’ part of the
>> equation —
>> shouldn’t p2m_access_n end up with a not-present p2m entry?
> SVM dosen't explicitly provide a read access bit so we treat
> read and
> write the same way.
Read and write can't possibly be treated the same. You ought to
use
the present bit to deny read (really: any) access, as also
implied by
George's response.
>>> They aren't treated the same as far sending out a vm_event goes.
>>> However, if we understand this correctly, there is no way to cause
>>> only
>>> read, or only write exits for NPT. They are bundled together under
>>> _PAGE_RW.
>>>
>>> So svm_do_nested_pgfault() tries to sort these out:
>>>
>>> 1781 struct npfec npfec = {
>>> 1782 .read_access = !(pfec & PFEC_insn_fetch),
>>> 1783 .write_access = !!(pfec & PFEC_write_access),
>>> 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
>>> 1785 .present = !!(pfec & PFEC_page_present),
>>> 1786 };
>>> 1787
>>> 1788 /* These bits are mutually exclusive */
>>> 1789 if ( pfec & NPT_PFEC_with_gla )
>>> 1790 npfec.kind = npfec_kind_with_gla;
>>> 1791 else if ( pfec & NPT_PFEC_in_gpt )
>>> 1792 npfec.kind = npfec_kind_in_gpt;
>>> 1793
>>> 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>>>
>>> but a read access is considered to be something that's not an insn
>>> fetch, and we only have a specific bit set for the write.
>>>
>>> Since hvm_hap_nested_page_fault() looks at npfec to decide when to
>>> send
>>> out a vm_event, this takes care of handling reads and writes
>>> differently
>>> at this level; however it's not possible to set separate single
>>> "don't
>>> read" or "don't write" exit-causing flags with NPT.
>> All fine, but George's question was raised in the context of
>> permission
>> conversion from p2m to pte representation.
>
> I've tried a test with xen access that sets XENMEM_access_n on all the
> pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
> the p2m_access_n case and the guest fails with triple fault. There are
> a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
> invalid if the flag is not present. I don't think this is the way to go
> with the p2m_access_n flags.
I will absolutely nack any interface where if the caller says, "Please
remove read permission", the hypervisor says, "OK!" but then allows read
permission anyway -- particularly in one which is allegedly designed for
security tools.
If it's not practical / more work than it's worth doing at the moment to
implement p2m_access_n on NPT, then you should return an error when it's
requested.
The same really should be true for write-only permission as well -- if
it's not possible to allow writes but not reads, then you should return
an error when such permissions are requested.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
>>> Tamas K Lengyel 07/19/18 5:09 PM >>> >On Thu, Jul 19, 2018 at 2:30 AM Jan Beulich wrote: > >>> On 19.07.18 at 10:18, wrote: > > On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote: > >> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila > >> > +break; >> >> > +case p2m_access_x: >> >> > +flags &= ~_PAGE_RW; >> >> > +break; >> >> > +case p2m_access_rwx: >> >> > +default: >> >> > +break; >> >> > } >> >> I think you want another blank line here too. >> >> >> >> Also, this doesn’t seem to capture the ‘r’ part of the equation — >> >> shouldn’t p2m_access_n end up with a not-present p2m entry? >> > >> > SVM dosen't explicitly provide a read access bit so we treat read and >> > write the same way. >> >> Read and write can't possibly be treated the same. You ought to use >> the present bit to deny read (really: any) access, as also implied by >> George's response. > >We already treat write accesses also as read on Intel because of >hardware limitations with CMPXCHG. So I don't see a problem with this. Right - write implies read. Which means no-read implies no-write. Which still means to me that p2m_access_n can't result in other than a not- present entry. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On Thu, Jul 19, 2018 at 2:30 AM Jan Beulich wrote:
>
> >>> On 19.07.18 at 10:18, wrote:
> > On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
> >> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila >> > @@ -112,8 +117,37 @@ static unsigned long p2m_type_to_flags(const
> >> > struct p2m_domain *p2m,
> >> > flags |= _PAGE_PWT;
> >> > ASSERT(!level);
> >> > }
> >> > -return flags | P2M_BASE_FLAGS | _PAGE_PCD;
> >> > +flags |= P2M_BASE_FLAGS | _PAGE_PCD;
> >> > +break;
> >> > +}
> >> I think you want a blank line here.
> >>
> >> >
> >> > +switch ( access )
> >> > +{
> >> > +case p2m_access_r:
> >> > +case p2m_access_w:
> >> > +flags |= _PAGE_NX_BIT;
> >> > +flags &= ~_PAGE_RW;
> >> > +break;
> >> > +case p2m_access_rw:
> >> > +flags |= _PAGE_NX_BIT;
> >> > +break;
> >> > +case p2m_access_n:
> >> > +case p2m_access_n2rwx:
> >> > +flags |= _PAGE_NX_BIT;
> >> > +flags &= ~_PAGE_RW;
> >> > +break;
> >> > +case p2m_access_rx:
> >> > +case p2m_access_wx:
> >> > +case p2m_access_rx2rw:
> >> > +flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
> >> This looks like a mistake — this will unconditionally enable
> >> execution, even if the underlying p2m type forbids it.
> >> ept_p2m_type_to_flags() doesn’t do that.
> >>
> >> >
> >> > +break;
> >> > +case p2m_access_x:
> >> > +flags &= ~_PAGE_RW;
> >> > +break;
> >> > +case p2m_access_rwx:
> >> > +default:
> >> > +break;
> >> > }
> >> I think you want another blank line here too.
> >>
> >> Also, this doesn’t seem to capture the ‘r’ part of the equation —
> >> shouldn’t p2m_access_n end up with a not-present p2m entry?
> >
> > SVM dosen't explicitly provide a read access bit so we treat read and
> > write the same way.
>
> Read and write can't possibly be treated the same. You ought to use
> the present bit to deny read (really: any) access, as also implied by
> George's response.
We already treat write accesses also as read on Intel because of
hardware limitations with CMPXCHG. So I don't see a problem with this.
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 19.07.18 at 10:43, wrote:
> > On 07/19/2018 11:30 AM, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On 19.07.18 at 10:18, wrote:
> > > > On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
> > > > >
> > > > > >
> > > > > > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila > > > > > fender.c
> > > > > > +break;
> > > > > > +case p2m_access_x:
> > > > > > +flags &= ~_PAGE_RW;
> > > > > > +break;
> > > > > > +case p2m_access_rwx:
> > > > > > +default:
> > > > > > +break;
> > > > > > }
> > > > > I think you want another blank line here too.
> > > > >
> > > > > Also, this doesn’t seem to capture the ‘r’ part of the
> > > > > equation —
> > > > > shouldn’t p2m_access_n end up with a not-present p2m entry?
> > > > SVM dosen't explicitly provide a read access bit so we treat
> > > > read and
> > > > write the same way.
> > > Read and write can't possibly be treated the same. You ought to
> > > use
> > > the present bit to deny read (really: any) access, as also
> > > implied by
> > > George's response.
> > They aren't treated the same as far sending out a vm_event goes.
> > However, if we understand this correctly, there is no way to cause
> > only
> > read, or only write exits for NPT. They are bundled together under
> > _PAGE_RW.
> >
> > So svm_do_nested_pgfault() tries to sort these out:
> >
> > 1781 struct npfec npfec = {
> > 1782 .read_access = !(pfec & PFEC_insn_fetch),
> > 1783 .write_access = !!(pfec & PFEC_write_access),
> > 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
> > 1785 .present = !!(pfec & PFEC_page_present),
> > 1786 };
> > 1787
> > 1788 /* These bits are mutually exclusive */
> > 1789 if ( pfec & NPT_PFEC_with_gla )
> > 1790 npfec.kind = npfec_kind_with_gla;
> > 1791 else if ( pfec & NPT_PFEC_in_gpt )
> > 1792 npfec.kind = npfec_kind_in_gpt;
> > 1793
> > 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
> >
> > but a read access is considered to be something that's not an insn
> > fetch, and we only have a specific bit set for the write.
> >
> > Since hvm_hap_nested_page_fault() looks at npfec to decide when to
> > send
> > out a vm_event, this takes care of handling reads and writes
> > differently
> > at this level; however it's not possible to set separate single
> > "don't
> > read" or "don't write" exit-causing flags with NPT.
> All fine, but George's question was raised in the context of
> permission
> conversion from p2m to pte representation.
I've tried a test with xen access that sets XENMEM_access_n on all the
pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
the p2m_access_n case and the guest fails with triple fault. There are
a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
invalid if the flag is not present. I don't think this is the way to go
with the p2m_access_n flags.
Thanks,
Alex
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
>>> On 19.07.18 at 10:43, wrote:
> On 07/19/2018 11:30 AM, Jan Beulich wrote:
> On 19.07.18 at 10:18, wrote:
>>> On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila +break;
> +case p2m_access_x:
> +flags &= ~_PAGE_RW;
> +break;
> +case p2m_access_rwx:
> +default:
> +break;
> }
I think you want another blank line here too.
Also, this doesn’t seem to capture the ‘r’ part of the equation —
shouldn’t p2m_access_n end up with a not-present p2m entry?
>>>
>>> SVM dosen't explicitly provide a read access bit so we treat read and
>>> write the same way.
>>
>> Read and write can't possibly be treated the same. You ought to use
>> the present bit to deny read (really: any) access, as also implied by
>> George's response.
>
> They aren't treated the same as far sending out a vm_event goes.
> However, if we understand this correctly, there is no way to cause only
> read, or only write exits for NPT. They are bundled together under _PAGE_RW.
>
> So svm_do_nested_pgfault() tries to sort these out:
>
> 1781 struct npfec npfec = {
> 1782 .read_access = !(pfec & PFEC_insn_fetch),
> 1783 .write_access = !!(pfec & PFEC_write_access),
> 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
> 1785 .present = !!(pfec & PFEC_page_present),
> 1786 };
> 1787
> 1788 /* These bits are mutually exclusive */
> 1789 if ( pfec & NPT_PFEC_with_gla )
> 1790 npfec.kind = npfec_kind_with_gla;
> 1791 else if ( pfec & NPT_PFEC_in_gpt )
> 1792 npfec.kind = npfec_kind_in_gpt;
> 1793
> 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>
> but a read access is considered to be something that's not an insn
> fetch, and we only have a specific bit set for the write.
>
> Since hvm_hap_nested_page_fault() looks at npfec to decide when to send
> out a vm_event, this takes care of handling reads and writes differently
> at this level; however it's not possible to set separate single "don't
> read" or "don't write" exit-causing flags with NPT.
All fine, but George's question was raised in the context of permission
conversion from p2m to pte representation.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On 07/19/2018 11:30 AM, Jan Beulich wrote:
On 19.07.18 at 10:18, wrote:
>> On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
On Jul 2, 2018, at 8:42 AM, Alexandru Isaila >>> +break;
+case p2m_access_x:
+flags &= ~_PAGE_RW;
+break;
+case p2m_access_rwx:
+default:
+break;
}
>>> I think you want another blank line here too.
>>>
>>> Also, this doesn’t seem to capture the ‘r’ part of the equation —
>>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>>
>> SVM dosen't explicitly provide a read access bit so we treat read and
>> write the same way.
>
> Read and write can't possibly be treated the same. You ought to use
> the present bit to deny read (really: any) access, as also implied by
> George's response.
They aren't treated the same as far sending out a vm_event goes.
However, if we understand this correctly, there is no way to cause only
read, or only write exits for NPT. They are bundled together under _PAGE_RW.
So svm_do_nested_pgfault() tries to sort these out:
1781 struct npfec npfec = {
1782 .read_access = !(pfec & PFEC_insn_fetch),
1783 .write_access = !!(pfec & PFEC_write_access),
1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
1785 .present = !!(pfec & PFEC_page_present),
1786 };
1787
1788 /* These bits are mutually exclusive */
1789 if ( pfec & NPT_PFEC_with_gla )
1790 npfec.kind = npfec_kind_with_gla;
1791 else if ( pfec & NPT_PFEC_in_gpt )
1792 npfec.kind = npfec_kind_in_gpt;
1793
1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
but a read access is considered to be something that's not an insn
fetch, and we only have a specific bit set for the write.
Since hvm_hap_nested_page_fault() looks at npfec to decide when to send
out a vm_event, this takes care of handling reads and writes differently
at this level; however it's not possible to set separate single "don't
read" or "don't write" exit-causing flags with NPT.
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
>>> On 19.07.18 at 10:18, wrote:
> On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
>> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila > > @@ -112,8 +117,37 @@ static unsigned long p2m_type_to_flags(const
>> > struct p2m_domain *p2m,
>> > flags |= _PAGE_PWT;
>> > ASSERT(!level);
>> > }
>> > -return flags | P2M_BASE_FLAGS | _PAGE_PCD;
>> > +flags |= P2M_BASE_FLAGS | _PAGE_PCD;
>> > +break;
>> > +}
>> I think you want a blank line here.
>>
>> >
>> > +switch ( access )
>> > +{
>> > +case p2m_access_r:
>> > +case p2m_access_w:
>> > +flags |= _PAGE_NX_BIT;
>> > +flags &= ~_PAGE_RW;
>> > +break;
>> > +case p2m_access_rw:
>> > +flags |= _PAGE_NX_BIT;
>> > +break;
>> > +case p2m_access_n:
>> > +case p2m_access_n2rwx:
>> > +flags |= _PAGE_NX_BIT;
>> > +flags &= ~_PAGE_RW;
>> > +break;
>> > +case p2m_access_rx:
>> > +case p2m_access_wx:
>> > +case p2m_access_rx2rw:
>> > +flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
>> This looks like a mistake — this will unconditionally enable
>> execution, even if the underlying p2m type forbids it.
>> ept_p2m_type_to_flags() doesn’t do that.
>>
>> >
>> > +break;
>> > +case p2m_access_x:
>> > +flags &= ~_PAGE_RW;
>> > +break;
>> > +case p2m_access_rwx:
>> > +default:
>> > +break;
>> > }
>> I think you want another blank line here too.
>>
>> Also, this doesn’t seem to capture the ‘r’ part of the equation —
>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>
> SVM dosen't explicitly provide a read access bit so we treat read and
> write the same way.
Read and write can't possibly be treated the same. You ought to use
the present bit to deny read (really: any) access, as also implied by
George's response.
Also - please trim the quoting of your replies.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On 07/19/2018 11:18 AM, Isaila Alexandru wrote:
> On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila >> om> wrote:
>>>
>>> From: Isaila Alexandru
>>>
>>> This patch adds access rights for the NPT pages. The access rights
>>> are
>>> saved in a radix tree with the root saved in p2m_domain. The rights
>>> are manipulated through p2m_set_access()
>>> and p2m_get_access() functions.
>>> The patch follows the ept logic.
>> This description needs to be much more complete. Something like
>> this:
>>
>> ---
>> This patch adds access control for NPT mode.
>>
>> There aren’t enough extra bits to store the access rights in the NPT
>> p2m table, so we add a radix tree to store the rights. For
>> efficiency, remove entries which match the default permissions rather
>> than continuing to store them.
>>
>> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking
>> an access, and removing / adding RW or NX flags as appropriate.
>> ---
>>
> I will update the patch description.
>>>
>>>
>>> Note: It was tested with xen-access write
>>>
>>> Signed-off-by: Alexandru Isaila
>>
>>>
>>>
>>> ---
>>> Changes since V2:
>>> - Delete blak line
>>> - Add return if p2m_access_rwx = a
>>> - Delete the comment from p2m_pt_get_entry()
>>> - Moved radix_tree_init() to arch_monitor_init_domain().
>>> ---
>>> xen/arch/x86/mm/mem_access.c | 3 ++
>>> xen/arch/x86/mm/p2m-pt.c | 109
>>> ++-
>>> xen/arch/x86/mm/p2m.c| 6 +++
>>> xen/arch/x86/monitor.c | 13 +
>>> xen/include/asm-x86/mem_access.h | 2 +-
>>> xen/include/asm-x86/p2m.h| 6 +++
>>> 6 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c
>>> b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..d78c82c 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa,
>>> unsigned long gla,
>>> {
>>> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>>> req->u.mem_access.gla = gla;
>>> +}
>>>
>>> +if ( npfec.gla_valid || cpu_has_svm )
>>> +{
>> I can’t immediately tell what this is about, which means it needs a
>> comment.
>>
>> It may also be (depending on why this is here) that “cpu_has_svm”
>> makes this more fragile — if this is really about having a radix
>> tree, for instance, then you should probably check for a radix tree.
>
> This is about the different npfec on SVN. The gla in never valid so the
> fault flag will not be set.
Specifically, this is what svm_do_nested_pgfault() does:
1781 struct npfec npfec = {
1782 .read_access = !(pfec & PFEC_insn_fetch),
1783 .write_access = !!(pfec & PFEC_write_access),
1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
1785 .present = !!(pfec & PFEC_page_present),
1786 };
1787
1788 /* These bits are mutually exclusive */
1789 if ( pfec & NPT_PFEC_with_gla )
1790 npfec.kind = npfec_kind_with_gla;
1791 else if ( pfec & NPT_PFEC_in_gpt )
1792 npfec.kind = npfec_kind_in_gpt;
1793
1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
so gla_valid is never non-0 on SVM.
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
On Mi, 2018-07-18 at 15:33 +, George Dunlap wrote:
>
> >
> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila > om> wrote:
> >
> > From: Isaila Alexandru
> >
> > This patch adds access rights for the NPT pages. The access rights
> > are
> > saved in a radix tree with the root saved in p2m_domain. The rights
> > are manipulated through p2m_set_access()
> > and p2m_get_access() functions.
> > The patch follows the ept logic.
> This description needs to be much more complete. Something like
> this:
>
> ---
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT
> p2m table, so we add a radix tree to store the rights. For
> efficiency, remove entries which match the default permissions rather
> than continuing to store them.
>
> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking
> an access, and removing / adding RW or NX flags as appropriate.
> ---
>
I will update the patch description.
> >
> >
> > Note: It was tested with xen-access write
> >
> > Signed-off-by: Alexandru Isaila
>
> >
> >
> > ---
> > Changes since V2:
> > - Delete blak line
> > - Add return if p2m_access_rwx = a
> > - Delete the comment from p2m_pt_get_entry()
> > - Moved radix_tree_init() to arch_monitor_init_domain().
> > ---
> > xen/arch/x86/mm/mem_access.c | 3 ++
> > xen/arch/x86/mm/p2m-pt.c | 109
> > ++-
> > xen/arch/x86/mm/p2m.c| 6 +++
> > xen/arch/x86/monitor.c | 13 +
> > xen/include/asm-x86/mem_access.h | 2 +-
> > xen/include/asm-x86/p2m.h| 6 +++
> > 6 files changed, 124 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_access.c
> > b/xen/arch/x86/mm/mem_access.c
> > index c0cd017..d78c82c 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa,
> > unsigned long gla,
> > {
> > req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> > req->u.mem_access.gla = gla;
> > +}
> >
> > +if ( npfec.gla_valid || cpu_has_svm )
> > +{
> I can’t immediately tell what this is about, which means it needs a
> comment.
>
> It may also be (depending on why this is here) that “cpu_has_svm”
> makes this more fragile — if this is really about having a radix
> tree, for instance, then you should probably check for a radix tree.
This is about the different npfec on SVN. The gla in never valid so the
fault flag will not be set.
>
> >
> > if ( npfec.kind == npfec_kind_with_gla )
> > req->u.mem_access.flags |=
> > MEM_ACCESS_FAULT_WITH_GLA;
> > else if ( npfec.kind == npfec_kind_in_gpt )
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> > index b8c5d2e..4330d1f 100644
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -68,7 +68,8 @@
> > static unsigned long p2m_type_to_flags(const struct p2m_domain
> > *p2m,
> > p2m_type_t t,
> > mfn_t mfn,
> > - unsigned int level)
> > + unsigned int level,
> > + p2m_access_t access)
> > {
> > unsigned long flags;
> > /*
> > @@ -87,23 +88,27 @@ static unsigned long p2m_type_to_flags(const
> > struct p2m_domain *p2m,
> > case p2m_ram_paged:
> > case p2m_ram_paging_in:
> > default:
> > -return flags | _PAGE_NX_BIT;
> > +flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
> > +break;
> > case p2m_grant_map_ro:
> > return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
> > case p2m_ioreq_server:
> > flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> > if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> > -return flags & ~_PAGE_RW;
> > -return flags;
> > +flags &= ~_PAGE_RW;
> > +break;
> > case p2m_ram_ro:
> > case p2m_ram_logdirty:
> > case p2m_ram_shared:
> > -return flags | P2M_BASE_FLAGS;
> > +flags |= P2M_BASE_FLAGS;
> > +break;
> > case p2m_ram_rw:
> > -return flags | P2M_BASE_FLAGS | _PAGE_RW;
> > +flags |= P2M_BASE_FLAGS | _PAGE_RW;
> > +break;
> > case p2m_grant_map_rw:
> > case p2m_map_foreign:
> > -return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> > +flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> > +break;
> > case p2m_mmio_direct:
> > if ( !rangeset_contains_singleton(mmio_ro_ranges,
> > mfn_x(mfn)) )
> > flags |= _PAGE_RW;
> > @@ -112,8 +117,37 @@ static unsigned long p2m_type_to_flags(const
> > struct p2m_domain *p2m,
> > flags |= _PAGE_PWT;
> > ASSERT(!level);
> > }
> > -return flags |
Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT
> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila wrote:
>
> From: Isaila Alexandru
>
> This patch adds access rights for the NPT pages. The access rights are
> saved in a radix tree with the root saved in p2m_domain. The rights are
> manipulated through p2m_set_access()
> and p2m_get_access() functions.
> The patch follows the ept logic.
This description needs to be much more complete. Something like this:
---
This patch adds access control for NPT mode.
There aren’t enough extra bits to store the access rights in the NPT p2m table,
so we add a radix tree to store the rights. For efficiency, remove entries
which match the default permissions rather than continuing to store them.
Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking an
access, and removing / adding RW or NX flags as appropriate.
---
>
> Note: It was tested with xen-access write
>
> Signed-off-by: Alexandru Isaila
>
> ---
> Changes since V2:
> - Delete blak line
> - Add return if p2m_access_rwx = a
> - Delete the comment from p2m_pt_get_entry()
> - Moved radix_tree_init() to arch_monitor_init_domain().
> ---
> xen/arch/x86/mm/mem_access.c | 3 ++
> xen/arch/x86/mm/p2m-pt.c | 109 ++-
> xen/arch/x86/mm/p2m.c| 6 +++
> xen/arch/x86/monitor.c | 13 +
> xen/include/asm-x86/mem_access.h | 2 +-
> xen/include/asm-x86/p2m.h| 6 +++
> 6 files changed, 124 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index c0cd017..d78c82c 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
> {
> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> req->u.mem_access.gla = gla;
> +}
>
> +if ( npfec.gla_valid || cpu_has_svm )
> +{
I can’t immediately tell what this is about, which means it needs a comment.
It may also be (depending on why this is here) that “cpu_has_svm” makes this
more fragile — if this is really about having a radix tree, for instance, then
you should probably check for a radix tree.
> if ( npfec.kind == npfec_kind_with_gla )
> req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> else if ( npfec.kind == npfec_kind_in_gpt )
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index b8c5d2e..4330d1f 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -68,7 +68,8 @@
> static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
>p2m_type_t t,
>mfn_t mfn,
> - unsigned int level)
> + unsigned int level,
> + p2m_access_t access)
> {
> unsigned long flags;
> /*
> @@ -87,23 +88,27 @@ static unsigned long p2m_type_to_flags(const struct
> p2m_domain *p2m,
> case p2m_ram_paged:
> case p2m_ram_paging_in:
> default:
> -return flags | _PAGE_NX_BIT;
> +flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
> +break;
> case p2m_grant_map_ro:
> return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
> case p2m_ioreq_server:
> flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> -return flags & ~_PAGE_RW;
> -return flags;
> +flags &= ~_PAGE_RW;
> +break;
> case p2m_ram_ro:
> case p2m_ram_logdirty:
> case p2m_ram_shared:
> -return flags | P2M_BASE_FLAGS;
> +flags |= P2M_BASE_FLAGS;
> +break;
> case p2m_ram_rw:
> -return flags | P2M_BASE_FLAGS | _PAGE_RW;
> +flags |= P2M_BASE_FLAGS | _PAGE_RW;
> +break;
> case p2m_grant_map_rw:
> case p2m_map_foreign:
> -return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> +flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
> +break;
> case p2m_mmio_direct:
> if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
> flags |= _PAGE_RW;
> @@ -112,8 +117,37 @@ static unsigned long p2m_type_to_flags(const struct
> p2m_domain *p2m,
> flags |= _PAGE_PWT;
> ASSERT(!level);
> }
> -return flags | P2M_BASE_FLAGS | _PAGE_PCD;
> +flags |= P2M_BASE_FLAGS | _PAGE_PCD;
> +break;
> +}
I think you want a blank line here.
> +switch ( access )
> +{
> +case p2m_access_r:
> +case p2m_access_w:
> +flags |= _PAGE_NX_BIT;
> +flags &= ~_PAGE_RW;
> +break;
> +case p2m_access_rw:
> +flags |= _PAGE_NX_BIT;
> +break;
> +case p2m_access_n:
> +case
