Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.07.18 at 10:18, <aisaila@xxxxxxxxxxxxxxx> wrote:
> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c 
>> > @@ -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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel