Re: [PATCH v3 04/12] KVM: x86: Add ioctl for accepting a userspace provided MSR list

[Alexander Graf <graf@amazon.com>]

Hi Jim,

On 20.08.20 19:30, Jim Mattson wrote:
> 
> 
> On Wed, Aug 19, 2020 at 2:00 AM Alexander Graf <graf@amazon.com> wrote:
> 
>> Why would we still need this with the allow list and user space #GP
>> deflection logic in place?
> 
> Conversion to an allow list is cumbersome when you have a short deny
> list. Suppose that I want to implement the following deny list:
> {IA32_ARCH_CAPABILITIES, HV_X64_MSR_REFERENCE_TSC,
> MSR_GOOGLE_TRUE_TIME, MSR_GOOGLE_FDR_TRACE, MSR_GOOGLE_HBI}. What
> would the corresponding deny list look like? Given your current
> implementation, I don't think the corresponding allow list can
> actually be constructed. I want to allow 2^32-5 MSRs, but I can allow
> at most 122880, if I've done the math correctly. (10 ranges, each
> spanning at most 0x600 bytes worth of bitmap.)

There are only very few MSR ranges that actually data. So in your case, 
to allow all MSRs that Linux knows about in msr-index.h, you would need

   [0x00000000 - 0x00002000]
   [0x40000000 - 0x40000200]
   [0x4b564d00 - 0x4b564e00]
   [0x80868000 - 0x80868020]
   [0xc0000000 - 0xc0000200]
   [0xc0010000 - 0xc0012000]
   [0xc0020000 - 0xc0020010]

which are 7 regions. For good measure, you can probably pad every one of 
them to the full 0x3000 MSRs they can span.

For MSRs that KVM actually handles in-kernel (others don't need to be 
allowed), the list shrinks to 5:

   [0x00000000 - 0x00001000]
   [0x40000000 - 0x40000200]
   [0x4b564d00 - 0x4b564e00]
   [0xc0000000 - 0xc0000200]
   [0xc0010000 - 0xc0012000]

Let's extend them a bit to make reasoning easier:

   [0x00000000 - 0x00003000]
   [0x40000000 - 0x40003000]
   [0x4b564d00 - 0x4b567000]
   [0xc0000000 - 0xc0003000]
   [0xc0010000 - 0xc0013000]

What are the odds that you will want to implicitly (without a new CAP, 
that would need user space adjustments anyway) have a random new MSR 
handled in-kernel with an identifier that is outside of those ranges?

I'm fairly confident that trends towards 0.

The only real downside I can see is that we just wasted ~8kb of RAM. 
Nothing I would really get hung up on though.

> Perhaps we should adopt allow/deny rules similar to those accepted by
> most firewalls. Instead of ports, we have MSR indices. Instead of
> protocols, we have READ, WRITE, or READ/WRITE. Suppose that we
> supported up to <n> rules of the form: {start index, end index, access
> modes, allow or deny}? Rules would be processed in the order given,
> and the first rule that matched a given access would take precedence.
> Finally, userspace could specify the default behavior (either allow or
> deny) for any MSR access that didn't match any of the rules.
> 
> Thoughts?

That wouldn't scale well if you want to allow all architecturally useful 
MSRs in a purely allow list fashion. You'd have to create hundreds of 
rules - or at least a few dozen if you combine contiguous ranges.

If you really desperately believe a deny list is a better fit for your 
use case, we could redesign the interface differently:

struct msr_set_accesslist {
#define MSR_ACCESSLIST_DEFAULT_ALLOW 0
#define MSR_ACCESSLIST_DEFAULT_DENY  1
     u32 flags;
     struct {
         u32 flags;
         u32 nmsrs; /* MSRs in bitmap */
         u32 base; /* first MSR address to bitmap */
         void *bitmap; /* pointer to bitmap, 1 means allow, 0 deny */
     } lists[10];
};

which means in your use case, you can do

u64 deny = 0;
struct msr_set_accesslist access = {
     .flags = MSR_ACCESSLIST_DEFAULT_ALLOW,
     .lists = {
         {
             .nmsrs = 1,
             .base = IA32_ARCH_CAPABILITIES,
             .bitmap = &deny,
         }, {
         {
             .nmsrs = 1,
             .base = HV_X64_MSR_REFERENCE_TSC,
             .bitmap = &deny,
         }, {
         {
             .nmsrs = 1,
             /* can probably be combined with the ones below? */
             .base = MSR_GOOGLE_TRUE_TIME,
             .bitmap = &deny,
         }, {
         {
             .nmsrs = 1,
             .base = MSR_GOOGLE_FDR_TRACE,
             .bitmap = &deny,
         }, {
         {
             .nmsrs = 1,
             .base = MSR_GOOGLE_HBI,
             .bitmap = &deny,
         },
     }
};

msr_set_accesslist(kvm_fd, &access);

while I can do the same dance as before, but with a single call rather 
than multiple ones.

What do you think?


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879