From: Jan Beulich <jbeulich@suse.com>
To: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
Cc: PetreOvidiu PIRCALABU <ppircalabu@bitdefender.com>,
"kevin.tian@intel.com" <kevin.tian@intel.com>,
"tamas@tklengyel.com" <tamas@tklengyel.com>,
"wl@xen.org" <wl@xen.org>,
"rcojocaru@bitdefender.com" <rcojocaru@bitdefender.com>,
"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
"jun.nakajima@intel.com" <jun.nakajima@intel.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2] x86/mm: Add mem access rights to NPT
Date: Mon, 2 Sep 2019 13:46:05 +0200 [thread overview]
Message-ID: <215e5edf-2105-c591-0615-457c798e3029@suse.com> (raw)
In-Reply-To: <0b745940-d71b-7ce3-37d2-7ae4ff9e5819@bitdefender.com>
On 02.09.2019 13:23, Alexandru Stefan ISAILA wrote:
> On 29.08.2019 18:04, Jan Beulich wrote:
>> On 22.08.2019 16:02, Alexandru Stefan ISAILA wrote:
>>> This patch adds access control for NPT mode.
>>>
>>> The access rights are stored in the NPT p2m table 56:53.
>>
>> Why starting from bit 53? I can't seem to find any use of bit 52.
>
> There is a comment in page.h that warns that bit 12(52) is taken.
> "/*
> * Bit 12 of a 24-bit flag mask. This corresponds to bit 52 of a pte.
> * This is needed to distinguish between user and kernel PTEs since
> _PAGE_USER
> * is asserted for both.
> */
> #define _PAGE_GUEST_KERNEL (1U<<12)
> "
But that's a PV-only thing. With sufficient care it should be
possible to have overlapping uses. And given that the available
bit are a pretty limited resource, I'd very much appreciate if
you at least tried to make this work.
>>> @@ -104,8 +112,32 @@ 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;
>>> }
>>> +
>>> + switch ( access )
>>> + {
>>> + case p2m_access_r:
>>> + flags |= _PAGE_NX_BIT;
>>> + flags &= ~_PAGE_RW;
>>> + break;
>>> + case p2m_access_rw:
>>> + flags |= _PAGE_NX_BIT;
>>> + break;
>>> + case p2m_access_rx:
>>> + case p2m_access_rx2rw:
>>> + flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
>>> + break;
>>> + case p2m_access_x:
>>> + flags &= ~_PAGE_RW;
>>> + break;
>>
>> I can't seem to be able to follow you here. In fact I don't see
>> how you would be able to express execute-only with NPT. If this
>> is really needed for some reason, then a justifying comment
>> should be added.
>
> Execute-only should be expressed as not PAGE_RW and PAGE_NX_BIT not set.
But that still doesn't yield exec-only. Where is this "should be
expressed" stated? I.e. on what basis is it tolerable to also allow
read access despite a request to the contrary?
>>> @@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>> return rc;
>>> }
>>>
>>> +static int p2m_pt_check_access(p2m_access_t p2ma)
>>> +{
>>> + switch ( p2ma )
>>> + {
>>> + case p2m_access_n:
>>> + case p2m_access_w:
>>> + case p2m_access_wx:
>>> + case p2m_access_n2rwx:
>>> + return -EINVAL;
>>
>> I'm not convinced EINVAL is appropriate here - the argument isn't
>> invalid, it's just that there's no way to represent it.
>
> Would EPERM be a better return here?
Quite a bit better, yes. But still not optimal, but I confess that
I also can't find an optimal one. EDOM would look to be suitable too,
if one was to ignore the "math" aspect of it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
prev parent reply other threads:[~2019-09-02 11:46 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-22 14:02 [Xen-devel] [PATCH v2] x86/mm: Add mem access rights to NPT Alexandru Stefan ISAILA
2019-08-29 15:04 ` Jan Beulich
2019-09-02 11:23 ` Alexandru Stefan ISAILA
2019-09-02 11:46 ` Jan Beulich [this message]
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=215e5edf-2105-c591-0615-457c798e3029@suse.com \
--to=jbeulich@suse.com \
--cc=aisaila@bitdefender.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).