Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/07/2015 03:04 PM, Lengyel, Tamas wrote:
> 
> 
> On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 07/06/2015 09:30 PM, Lengyel, Tamas wrote:
>     >     If you'd prefer that I do some ground work for the future
>     (i.e. rename
>     >     MEM_ACCESS constants, etc.), please let me know.
>     >
>     >
>     > Yeap, that sounds reasonable to me.
> 
>     Any objections to this renaming?
> 
>     151 #define MEM_ACCESS_EMULATE_NOWRITE       (1 << 7)
>     152 /*
>     153  * Data is being sent back to the hypervisor in the event response,
>     to be
>     154  * returned by the read function when emulating an instruction.
>     155  * This flag is only useful when combined with MEM_ACCESS_EMULATE or
>     156  * MEM_ACCESS_EMULATE_NOWRITE.
>     157  * The flag has been defined here because it is used with mem_access
>     158  * events, and so should not accidentaly overwrite one of the above.
>     159  */
>     160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8)
> 
>     Are there any other small changes you'd like to see in this patch?
> 
> 
> That should suffice - with the flag being move to the VM_EVENT_FLAG list
> and the bitshift adjusted accordingly.

Actually, thinking about this more, there is a small problem with the
vm_event.h code:

1417 void p2m_mem_access_emulate_check(struct vcpu *v,
1418                                   const vm_event_response_t *rsp)
1419 {
1420     /* Mark vcpu for skipping one instruction upon rescheduling. */
1421     if ( rsp->flags & MEM_ACCESS_EMULATE )
1422     {
1423         xenmem_access_t access;
1424         bool_t violation = 1;
1425         const struct vm_event_mem_access *data = &rsp->u.mem_access;
1426
1427         if ( p2m_get_mem_access(v->domain, data->gfn, &access) == 0 )
1428         {
1429             switch ( access )
1430             {
1431             case XENMEM_access_n:
1432             case XENMEM_access_n2rwx:
1433             default:
1434                 violation = data->flags & MEM_ACCESS_RWX;

So, in p2m_mem_access_emulate_check() MEM_ACCESS_EMULATE is checked for
in rsp->flags (_not_ data->flags), but then in vm_event.h:

 42 /*
 43  * VCPU_PAUSED in a request signals that the vCPU triggering the
event has been
 44  *  paused
 45  * VCPU_PAUSED in a response signals to unpause the vCPU
 46  */
 47 #define VM_EVENT_FLAG_VCPU_PAUSED        (1 << 0)
 48 /* Flags to aid debugging mem_event */
 49 #define VM_EVENT_FLAG_FOREIGN            (1 << 1)

[...]

126 /*
127  * mem_access flag definitions
128  *
129  * These flags are set only as part of a mem_event request.
130  *
131  * R/W/X: Defines the type of violation that has triggered the event
132  *        Multiple types can be set in a single violation!
133  * GLA_VALID: If the gla field holds a guest VA associated with the
event
134  * FAULT_WITH_GLA: If the violation was triggered by accessing gla
135  * FAULT_IN_GPT: If the violation was triggered during translating gla
136  */
137 #define MEM_ACCESS_R                    (1 << 0)
138 #define MEM_ACCESS_W                    (1 << 1)
139 #define MEM_ACCESS_X                    (1 << 2)
140 #define MEM_ACCESS_RWX                  (MEM_ACCESS_R | MEM_ACCESS_W
| MEM_ACCESS_X)
141 #define MEM_ACCESS_RW                   (MEM_ACCESS_R | MEM_ACCESS_W)
142 #define MEM_ACCESS_RX                   (MEM_ACCESS_R | MEM_ACCESS_X)
143 #define MEM_ACCESS_WX                   (MEM_ACCESS_W | MEM_ACCESS_X)
144 #define MEM_ACCESS_GLA_VALID            (1 << 3)
145 #define MEM_ACCESS_FAULT_WITH_GLA       (1 << 4)
146 #define MEM_ACCESS_FAULT_IN_GPT         (1 << 5)
147 /*
148  * The following flags can be set in the response.
149  *
150  * Emulate the fault-causing instruction (if set in the event
response flags).
151  * This will allow the guest to continue execution without lifting
the page
152  * access restrictions.
153  */
154 #define MEM_ACCESS_EMULATE              (1 << 6)
155 /*
156  * Same as MEM_ACCESS_EMULATE, but with write operations or operations
157  * potentially having side effects (like memory mapped or port I/O)
disabled.
158  */
159 #define MEM_ACCESS_EMULATE_NOWRITE      (1 << 7)

So VM_EVENT_FLAG_FOREIGN (1 << 1), and then MEM_ACCESS_EMULATE (1 << 6).
Now you're adding VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2), and if we're
not very careful, slowly but surely the VM_EVENT_FLAG_ constants will
crawl towards (1 << 6) and start overlapping with MEM_ACCESS_EMULATE,
MEM_ACCESS_EMULATE_NOWRITE and so on, because they're not clearly
#defined right after the rest of the VM_EVENT_FLAG_ ones, are called
MEM_ACCESS_<something> just like the mem_access event specific flags,
and they use bit shifts relative to the MEM_ACCESS_ ones as well. This
is in part what has caused my confusion here.

What do you think?


Thanks,
Razvan