| Message ID | 20181213211021.202476-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: Change offset in kvm_write_guest_offset_cached to unsigned | expand |
On 12/13/2018 01:10 PM, Jim Mattson wrote: > Avoid potential out-of-bounds writes due to negative offsets. Note > that all current call sites are fine. > > Fixes: 4ec6e8636256 ("kvm: Introduce kvm_write_guest_offset_cached()") > Reported-by: Cfir Cohen <cfir@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Cfir Cohen <cfir@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > include/linux/kvm_host.h | 3 ++- > virt/kvm/kvm_main.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index c926698040e0d..a03d5e264e5e7 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -694,7 +694,8 @@ int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data, > int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > void *data, unsigned long len); > int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > - void *data, int offset, unsigned long len); > + void *data, unsigned int offset, > + unsigned long len); > int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > gpa_t gpa, unsigned long len); > int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2679e476b6c39..065ee2fb40340 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1965,7 +1965,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > EXPORT_SYMBOL_GPL(kvm_gfn_to_hva_cache_init); > > int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > - void *data, int offset, unsigned long len) > + void *data, unsigned int offset, > + unsigned long len) > { > struct kvm_memslots *slots = kvm_memslots(kvm); > int r; Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On Thu, Dec 13, 2018 at 01:10:21PM -0800, Jim Mattson wrote: > Avoid potential out-of-bounds writes due to negative offsets. Note > that all current call sites are fine. Oof. It'd be helpful if the changelog went into a bit more detail on how an out-of-bounds write is possible, e.g. even just a small blurb that says offset is used to directly adjust the gpa/hva. It wasn't immediately obvious (to me) that a negative offset was bad because the existing bounds check makes it look ok at first glance... BUG_ON(len + offset > ghc->len); Tangentially related, seems like this should be: if (WARN_ON(...)) return -EFAULT; Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > Fixes: 4ec6e8636256 ("kvm: Introduce kvm_write_guest_offset_cached()") > Reported-by: Cfir Cohen <cfir@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Cfir Cohen <cfir@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > include/linux/kvm_host.h | 3 ++- > virt/kvm/kvm_main.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index c926698040e0d..a03d5e264e5e7 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -694,7 +694,8 @@ int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data, > int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > void *data, unsigned long len); > int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > - void *data, int offset, unsigned long len); > + void *data, unsigned int offset, > + unsigned long len); > int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > gpa_t gpa, unsigned long len); > int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2679e476b6c39..065ee2fb40340 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1965,7 +1965,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > EXPORT_SYMBOL_GPL(kvm_gfn_to_hva_cache_init); > > int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > - void *data, int offset, unsigned long len) > + void *data, unsigned int offset, > + unsigned long len) > { > struct kvm_memslots *slots = kvm_memslots(kvm); > int r; > -- > 2.20.0.rc2.403.gdbc3b29805-goog >
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index c926698040e0d..a03d5e264e5e7 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -694,7 +694,8 @@ int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data, int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, void *data, unsigned long len); int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, - void *data, int offset, unsigned long len); + void *data, unsigned int offset, + unsigned long len); int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, gpa_t gpa, unsigned long len); int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2679e476b6c39..065ee2fb40340 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1965,7 +1965,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, EXPORT_SYMBOL_GPL(kvm_gfn_to_hva_cache_init); int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, - void *data, int offset, unsigned long len) + void *data, unsigned int offset, + unsigned long len) { struct kvm_memslots *slots = kvm_memslots(kvm); int r;
Avoid potential out-of-bounds writes due to negative offsets. Note that all current call sites are fine. Fixes: 4ec6e8636256 ("kvm: Introduce kvm_write_guest_offset_cached()") Reported-by: Cfir Cohen <cfir@google.com> Signed-off-by: Jim Mattson <jmattson@google.com> Reviewed-by: Cfir Cohen <cfir@google.com> Reviewed-by: Peter Shier <pshier@google.com> --- include/linux/kvm_host.h | 3 ++- virt/kvm/kvm_main.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)