Message ID | 20200320205546.2396-2-sean.j.christopherson@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: Fix memslot use-after-free bug | expand |
On Fri, Mar 20, 2020 at 01:55:40PM -0700, Sean Christopherson wrote: > Reset the LRU slot if it becomes invalid when deleting a memslot to fix > an out-of-bounds/use-after-free access when searching through memslots. > > Explicitly check for there being no used slots in search_memslots(), and > in the caller of s390's approximation variant. > > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots") > Reported-by: Qian Cai <cai@lca.pw> > Cc: Peter Xu <peterx@redhat.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/s390/kvm/kvm-s390.c | 3 +++ > include/linux/kvm_host.h | 3 +++ > virt/kvm/kvm_main.c | 3 +++ > 3 files changed, 9 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 807ed6d722dd..cb15fdda1fee 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args, > struct kvm_memslots *slots = kvm_memslots(kvm); > struct kvm_memory_slot *ms; > > + if (unlikely(!slots->used_slots)) > + return 0; > + > cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn); > ms = gfn_to_memslot(kvm, cur_gfn); > args->count = 0; > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 35bc52e187a2..b19dee4ed7d9 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) > int slot = atomic_read(&slots->lru_slot); > struct kvm_memory_slot *memslots = slots->memslots; > > + if (unlikely(!slots->used_slots)) > + return NULL; > + > if (gfn >= memslots[slot].base_gfn && > gfn < memslots[slot].base_gfn + memslots[slot].npages) > return &memslots[slot]; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 28eae681859f..f744bc603c53 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots, > > slots->used_slots--; > > + if (atomic_read(&slots->lru_slot) >= slots->used_slots) > + atomic_set(&slots->lru_slot, 0); Nit: could we drop the atomic ops? The "slots" is still only used in the current thread before the rcu assignment, so iirc it's fine (and RCU assignment should have a mem barrier when needed anyway). I thought resetting lru_slot to zero would be good enough when duplicating the slots... however if we want to do finer grained reset, maybe even better to reset also those invalidated ones (since we know this information)? if (slots->lru_slot >= slots->id_to_index[memslot->id]) slots->lru_slot = 0; Thanks, > + > for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) { > mslots[i] = mslots[i + 1]; > slots->id_to_index[mslots[i].id] = i; > -- > 2.24.1 >
On Fri, Mar 20, 2020 at 06:17:08PM -0400, Peter Xu wrote: > On Fri, Mar 20, 2020 at 01:55:40PM -0700, Sean Christopherson wrote: > > Reset the LRU slot if it becomes invalid when deleting a memslot to fix > > an out-of-bounds/use-after-free access when searching through memslots. > > > > Explicitly check for there being no used slots in search_memslots(), and > > in the caller of s390's approximation variant. > > > > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots") > > Reported-by: Qian Cai <cai@lca.pw> > > Cc: Peter Xu <peterx@redhat.com> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/s390/kvm/kvm-s390.c | 3 +++ > > include/linux/kvm_host.h | 3 +++ > > virt/kvm/kvm_main.c | 3 +++ > > 3 files changed, 9 insertions(+) > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > index 807ed6d722dd..cb15fdda1fee 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args, > > struct kvm_memslots *slots = kvm_memslots(kvm); > > struct kvm_memory_slot *ms; > > > > + if (unlikely(!slots->used_slots)) > > + return 0; > > + > > cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn); > > ms = gfn_to_memslot(kvm, cur_gfn); > > args->count = 0; > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 35bc52e187a2..b19dee4ed7d9 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) > > int slot = atomic_read(&slots->lru_slot); > > struct kvm_memory_slot *memslots = slots->memslots; > > > > + if (unlikely(!slots->used_slots)) > > + return NULL; > > + > > if (gfn >= memslots[slot].base_gfn && > > gfn < memslots[slot].base_gfn + memslots[slot].npages) > > return &memslots[slot]; > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 28eae681859f..f744bc603c53 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots, > > > > slots->used_slots--; > > > > + if (atomic_read(&slots->lru_slot) >= slots->used_slots) > > + atomic_set(&slots->lru_slot, 0); > > Nit: could we drop the atomic ops? The "slots" is still only used in > the current thread before the rcu assignment, so iirc it's fine (and > RCU assignment should have a mem barrier when needed anyway). No, atomic_t wraps an int inside a struct to prevent direct usage, e.g. virt/kvm/kvm_main.c: In function ‘kvm_memslot_delete’: virt/kvm/kvm_main.c:885:22: error: invalid operands to binary >= (have ‘atomic_t {aka struct <anonymous>}’ and ‘int’) if (slots->lru_slot >= slots->used_slots) ^ virt/kvm/kvm_main.c:886:19: error: incompatible types when assigning to type ‘atomic_t {aka struct <anonymous>}’ from type ‘int’ slots->lru_slot = 0; Buy yeah, the compiler barrier associated with atomic_read() isn't necessary. > I thought resetting lru_slot to zero would be good enough when > duplicating the slots... however if we want to do finer grained reset, > maybe even better to reset also those invalidated ones (since we know > this information)? > > if (slots->lru_slot >= slots->id_to_index[memslot->id]) > slots->lru_slot = 0; I'd prefer to go with the more obviously correct version. This code will rarely trigger, e.g. larger slots have lower indices and are more likely to be the LRU (by virtue of being the biggest), and dynamic memslot deletion is usually for relatively small ranges that are being remapped by the guest. > Thanks, > > > + > > for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) { > > mslots[i] = mslots[i + 1]; > > slots->id_to_index[mslots[i].id] = i; > > -- > > 2.24.1 > > > > -- > Peter Xu >
On Fri, Mar 20, 2020 at 03:40:41PM -0700, Sean Christopherson wrote: > On Fri, Mar 20, 2020 at 06:17:08PM -0400, Peter Xu wrote: > > On Fri, Mar 20, 2020 at 01:55:40PM -0700, Sean Christopherson wrote: > > > Reset the LRU slot if it becomes invalid when deleting a memslot to fix > > > an out-of-bounds/use-after-free access when searching through memslots. > > > > > > Explicitly check for there being no used slots in search_memslots(), and > > > in the caller of s390's approximation variant. > > > > > > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots") > > > Reported-by: Qian Cai <cai@lca.pw> > > > Cc: Peter Xu <peterx@redhat.com> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > arch/s390/kvm/kvm-s390.c | 3 +++ > > > include/linux/kvm_host.h | 3 +++ > > > virt/kvm/kvm_main.c | 3 +++ > > > 3 files changed, 9 insertions(+) > > > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > > index 807ed6d722dd..cb15fdda1fee 100644 > > > --- a/arch/s390/kvm/kvm-s390.c > > > +++ b/arch/s390/kvm/kvm-s390.c > > > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args, > > > struct kvm_memslots *slots = kvm_memslots(kvm); > > > struct kvm_memory_slot *ms; > > > > > > + if (unlikely(!slots->used_slots)) > > > + return 0; > > > + > > > cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn); > > > ms = gfn_to_memslot(kvm, cur_gfn); > > > args->count = 0; > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > index 35bc52e187a2..b19dee4ed7d9 100644 > > > --- a/include/linux/kvm_host.h > > > +++ b/include/linux/kvm_host.h > > > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) > > > int slot = atomic_read(&slots->lru_slot); > > > struct kvm_memory_slot *memslots = slots->memslots; > > > > > > + if (unlikely(!slots->used_slots)) > > > + return NULL; > > > + > > > if (gfn >= memslots[slot].base_gfn && > > > gfn < memslots[slot].base_gfn + memslots[slot].npages) > > > return &memslots[slot]; > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index 28eae681859f..f744bc603c53 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots, > > > > > > slots->used_slots--; > > > > > > + if (atomic_read(&slots->lru_slot) >= slots->used_slots) > > > + atomic_set(&slots->lru_slot, 0); > > > > Nit: could we drop the atomic ops? The "slots" is still only used in > > the current thread before the rcu assignment, so iirc it's fine (and > > RCU assignment should have a mem barrier when needed anyway). > > No, atomic_t wraps an int inside a struct to prevent direct usage, e.g. > > virt/kvm/kvm_main.c: In function ‘kvm_memslot_delete’: > virt/kvm/kvm_main.c:885:22: error: invalid operands to binary >= (have ‘atomic_t {aka struct <anonymous>}’ and ‘int’) > if (slots->lru_slot >= slots->used_slots) > ^ > virt/kvm/kvm_main.c:886:19: error: incompatible types when assigning to type ‘atomic_t {aka struct <anonymous>}’ from type ‘int’ > slots->lru_slot = 0; > > > Buy yeah, the compiler barrier associated with atomic_read() isn't > necessary. Oops, sorry. I was obviously thinking about QEMU's atomic helpers. > > > I thought resetting lru_slot to zero would be good enough when > > duplicating the slots... however if we want to do finer grained reset, > > maybe even better to reset also those invalidated ones (since we know > > this information)? > > > > if (slots->lru_slot >= slots->id_to_index[memslot->id]) > > slots->lru_slot = 0; > > I'd prefer to go with the more obviously correct version. This code will > rarely trigger, e.g. larger slots have lower indices and are more likely to > be the LRU (by virtue of being the biggest), and dynamic memslot deletion > is usually for relatively small ranges that are being remapped by the guest. IMHO the more obvious correct version could be unconditionally setting lru_slot to something invalid (e.g. -1) in kvm_dup_memslots(), then in the two search_memslots() skip the cache if lru_slot is invalid. That's IMHO easier to understand than the "!slots->used_slots" checks. But I've no strong opinion. Thanks,
On Fri, Mar 20, 2020 at 06:58:02PM -0400, Peter Xu wrote: > On Fri, Mar 20, 2020 at 03:40:41PM -0700, Sean Christopherson wrote: > > I thought resetting lru_slot to zero would be good enough when > > > duplicating the slots... however if we want to do finer grained reset, > > > maybe even better to reset also those invalidated ones (since we know > > > this information)? > > > > > > if (slots->lru_slot >= slots->id_to_index[memslot->id]) > > > slots->lru_slot = 0; > > > > I'd prefer to go with the more obviously correct version. This code will > > rarely trigger, e.g. larger slots have lower indices and are more likely to > > be the LRU (by virtue of being the biggest), and dynamic memslot deletion > > is usually for relatively small ranges that are being remapped by the guest. > > IMHO the more obvious correct version could be unconditionally setting > lru_slot to something invalid (e.g. -1) in kvm_dup_memslots(), then in > the two search_memslots() skip the cache if lru_slot is invalid. > That's IMHO easier to understand than the "!slots->used_slots" checks. > But I've no strong opinion. We'd still need the !slots->used_slots check. I considered adding an explicit check on @slot for the cache lookup, e.g. static inline struct kvm_memory_slot * search_memslots(struct kvm_memslots *slots, gfn_t gfn) { int start = 0, end = slots->used_slots; int slot = atomic_read(&slots->lru_slot); struct kvm_memory_slot *memslots = slots->memslots; if (likely(slot < slots->used_slots) && gfn >= memslots[slot].base_gfn && gfn < memslots[slot].base_gfn + memslots[slot].npages) return &memslots[slot]; But then the back half of the function still ends up with an out-of-bounds bug. E.g. if used_slots == 0, then start==0, and memslots[start].base_gfn accesses garbage. while (start < end) { slot = start + (end - start) / 2; if (gfn >= memslots[slot].base_gfn) end = slot; else start = slot + 1; } if (gfn >= memslots[start].base_gfn && gfn < memslots[start].base_gfn + memslots[start].npages) { atomic_set(&slots->lru_slot, start); return &memslots[start]; } return NULL; } I also didn't want to invalidate the lru_slot any more than is necessary, not that I'd expect it to make a noticeable difference even in a pathalogical scenario, but it seemed prudent.
On 20.03.20 21:55, Sean Christopherson wrote: > Reset the LRU slot if it becomes invalid when deleting a memslot to fix > an out-of-bounds/use-after-free access when searching through memslots. > > Explicitly check for there being no used slots in search_memslots(), and > in the caller of s390's approximation variant. > > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots") > Reported-by: Qian Cai <cai@lca.pw> > Cc: Peter Xu <peterx@redhat.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/s390/kvm/kvm-s390.c | 3 +++ > include/linux/kvm_host.h | 3 +++ > virt/kvm/kvm_main.c | 3 +++ > 3 files changed, 9 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 807ed6d722dd..cb15fdda1fee 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args, Adding Claudio, but > struct kvm_memslots *slots = kvm_memslots(kvm); > struct kvm_memory_slot *ms; > > + if (unlikely(!slots->used_slots)) > + return 0; > + this looks sane and like the right fix. Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn); > ms = gfn_to_memslot(kvm, cur_gfn); > args->count = 0; > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 35bc52e187a2..b19dee4ed7d9 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) > int slot = atomic_read(&slots->lru_slot); > struct kvm_memory_slot *memslots = slots->memslots; > > + if (unlikely(!slots->used_slots)) > + return NULL; > + > if (gfn >= memslots[slot].base_gfn && > gfn < memslots[slot].base_gfn + memslots[slot].npages) > return &memslots[slot]; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 28eae681859f..f744bc603c53 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots, > > slots->used_slots--; > > + if (atomic_read(&slots->lru_slot) >= slots->used_slots) > + atomic_set(&slots->lru_slot, 0); > + > for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) { > mslots[i] = mslots[i + 1]; > slots->id_to_index[mslots[i].id] = i; >
On Tue, 24 Mar 2020 08:12:59 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 20.03.20 21:55, Sean Christopherson wrote: > > Reset the LRU slot if it becomes invalid when deleting a memslot to > > fix an out-of-bounds/use-after-free access when searching through > > memslots. > > > > Explicitly check for there being no used slots in > > search_memslots(), and in the caller of s390's approximation > > variant. > > > > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on > > number of used slots") Reported-by: Qian Cai <cai@lca.pw> > > Cc: Peter Xu <peterx@redhat.com> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/s390/kvm/kvm-s390.c | 3 +++ > > include/linux/kvm_host.h | 3 +++ > > virt/kvm/kvm_main.c | 3 +++ > > 3 files changed, 9 insertions(+) > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > index 807ed6d722dd..cb15fdda1fee 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, > > struct kvm_s390_cmma_log *args, > > Adding Claudio, but > > struct kvm_memslots *slots = kvm_memslots(kvm); > > struct kvm_memory_slot *ms; > > > > + if (unlikely(!slots->used_slots)) > > + return 0; > > + this should never happen, as this function is only called during migration, and if we don't have any memory slots, then we will not try to migrate them. But this is something that is triggered by userspace, so we need to protect the kernel from rogue or broken userspace. Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > this looks sane and like the right fix. > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn); > > ms = gfn_to_memslot(kvm, cur_gfn); > > args->count = 0; > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 35bc52e187a2..b19dee4ed7d9 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, > > gfn_t gfn) int slot = atomic_read(&slots->lru_slot); > > struct kvm_memory_slot *memslots = slots->memslots; > > > > + if (unlikely(!slots->used_slots)) > > + return NULL; > > + > > if (gfn >= memslots[slot].base_gfn && > > gfn < memslots[slot].base_gfn + memslots[slot].npages) > > return &memslots[slot]; > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 28eae681859f..f744bc603c53 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct > > kvm_memslots *slots, > > slots->used_slots--; > > > > + if (atomic_read(&slots->lru_slot) >= slots->used_slots) > > + atomic_set(&slots->lru_slot, 0); > > + > > for (i = slots->id_to_index[memslot->id]; i < > > slots->used_slots; i++) { mslots[i] = mslots[i + 1]; > > slots->id_to_index[mslots[i].id] = i; > >
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 807ed6d722dd..cb15fdda1fee 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args, struct kvm_memslots *slots = kvm_memslots(kvm); struct kvm_memory_slot *ms; + if (unlikely(!slots->used_slots)) + return 0; + cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn); ms = gfn_to_memslot(kvm, cur_gfn); args->count = 0; diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 35bc52e187a2..b19dee4ed7d9 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) int slot = atomic_read(&slots->lru_slot); struct kvm_memory_slot *memslots = slots->memslots; + if (unlikely(!slots->used_slots)) + return NULL; + if (gfn >= memslots[slot].base_gfn && gfn < memslots[slot].base_gfn + memslots[slot].npages) return &memslots[slot]; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 28eae681859f..f744bc603c53 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots, slots->used_slots--; + if (atomic_read(&slots->lru_slot) >= slots->used_slots) + atomic_set(&slots->lru_slot, 0); + for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) { mslots[i] = mslots[i + 1]; slots->id_to_index[mslots[i].id] = i;
Reset the LRU slot if it becomes invalid when deleting a memslot to fix an out-of-bounds/use-after-free access when searching through memslots. Explicitly check for there being no used slots in search_memslots(), and in the caller of s390's approximation variant. Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots") Reported-by: Qian Cai <cai@lca.pw> Cc: Peter Xu <peterx@redhat.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/s390/kvm/kvm-s390.c | 3 +++ include/linux/kvm_host.h | 3 +++ virt/kvm/kvm_main.c | 3 +++ 3 files changed, 9 insertions(+)