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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jul 19, 2018 at 2:30 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel