Re: [PATCH v2 3/3] KVM: Take gpa_t in kvm_vcpu_map[_readonly]()

[Yosry Ahmed <yosry@kernel.org>]

> > > To elaborate a bit, I'm all for adding WARNs in flows where something bad is all
> > > but guaranteed to happen if an assumption is violated, or in APIs where there's
> > > a history of goofs and/or subtlety in how the API behaves.
> > >
> > > What I'm against is adding WARNs because someone could write bad code in the
> > > future, or because KVM doesn't do XYZ at this time.  Such WARNs usualy just add
> > > noise, and can even be actively harmful.  E.g. in this case, ignoring the PID
> > > usage, a reader might look at the WARN and think it's _wrong_ to map a page in
> > > order to access a subset of the page, which is just not true.
> > 
> > Yeah I agree with most/all of your objections to my suggestions, it's
> > usually that I don't have enough context to understand how the WARN
> > could be harmful (like here), or am just being too paranoid or
> > defending against bad code as you mentioned. I was mentioning your
> > objections semi-sarcastically and intentionally bringing up the WARN
> > in case it's actually useful.
> > 
> > Taking a step back, what I really want to clarify and/or detect misuse
> > of, is that kvm_vcpu_map() will map exactly one page, the one that the
> > GPA lies in. For example, there's nothing protecting against the PID
> > address being the last byte of the page, 
> 
> Well, technically there is:
> 
> 	    CC(!kvm_vcpu_is_legal_aligned_gpa(vcpu, vmcs12->posted_intr_desc_addr, 64))))
> 		return -EINVAL;
> 
> But I'm pretty sure what you're saying is that "nothing in the common helper code
> protects against a stupid caller".

Yes. Now I am actually glad I brought up the WARN and you elaborated
your thoughts, because it made me think and spell out my actual concern
(that my brain translated into just WARN initiailly): we need bounds
checking.

> 
> > in which case accessing all of it would be wrong as it spans the mapped page
> > boundary. This is difficult to hit if you are passing in a GFN, as it's more
> > obvious that KVM is mapping one physical page.
> 
> Eh, that just leads to a different class of bugs (and possible even *worse* bugs).
> E.g. caller passes in a GFN, accesses the kernel mapping beyond the page, and
> reads/write arbitrary kernel memory (pfn_valid() case using the direct map), or
> hits a !PRESENT #PF (remap case).
> 
> > Perhaps we just need to rename the functions (e.g.
> > kvm_vcpu_map_page()), or more intrusively pass in a size and do bounds
> > checking.
> 
> Definitely the latter.  Or both I guess, but probably just the latter.

I think both. I think renaming to kvm_vcpu_map_page() (and similar for
others) would further clarify things, especially with the introduction
of kvm_vcpu_map_ptr() below.

> 
> Commit 025dde582bbf ("KVM: Harden guest memory APIs against out-of-bounds accesses")
> added that type of hardening for the "slow" APIs, exactly because of the type of
> OOB bug you're describing: commit f559b2e9c5c5 ("KVM: nSVM: Ignore nCR3[4:0] when
> loading PDPTEs from memory").
> 
> Actually, that's actually useful feedback for patch 3.  __kvm_vcpu_map() should
> do the GPA=>GFN conversion, not its caller.

Yes.

> 
> Anyways, back to the hardening.  We can do it with minimal additional churn.  After
> patch 3 (passing a @gpa to __kvm_vcpu_map(), not a @gfn), do the below over a few
> patches (completely untested).  This way the common case of mapping and accessing
> an entire page Just Works, and flows like the PI descriptor handling don't have to
> many provide the length (which also can be error prone).

Yeah probably this (maybe not in the same order):
- Convert map->pfn to map->hpa.
- Pass size to __kvm_vcpu_map() and do bounds checking.
- Rename kvm_vcpu_map() and __kvm_vpcu_map() to kvm_vcpu_map_page() and
  __kvm_vcpu_map_page().
- Introduce kvm_vcpu_map_ptr() wrapper and simplify the nested PID call
  site.

Generally looks good with a small nit/question below. Peter, would you
be interested in extending the series to do this? If not, I can send a
follow up on top of your series when it's hashed out.

[..]
> @@ -1893,22 +1894,31 @@ static inline hpa_t pfn_to_hpa(kvm_pfn_t pfn)
>  	return (hpa_t)pfn << PAGE_SHIFT;
>  }
>  
> -int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
> -		   bool writable);
> +int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, gpa_t len,
> +		   struct kvm_host_map *map, bool writable);
>  void kvm_vcpu_unmap(struct kvm_vcpu *vcpu, struct kvm_host_map *map);
>  
>  static inline int kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa,
>  			       struct kvm_host_map *map)
>  {
> -	return __kvm_vcpu_map(vcpu, gpa_to_gfn(gpa), map, true);
> +	return __kvm_vcpu_map(vcpu, gpa, PAGE_SIZE, map, true);
>  }
>  
>  static inline int kvm_vcpu_map_readonly(struct kvm_vcpu *vcpu, gpa_t gpa,
>  					struct kvm_host_map *map)
>  {
> -	return __kvm_vcpu_map(vcpu, gpa_to_gfn(gpa), map, false);
> +	return __kvm_vcpu_map(vcpu, gpa, PAGE_SIZE, map, false);
>  }
>  
> +#define kvm_vcpu_map_ptr(__vcpu, __gpa, __ptr, __map)				\
> +({										\
> +	int r;									\
> +										\
> +	r = __kvm_vcpu_map(__vcpu, __gpa, sizeof(*(__ptr)), __map, true);	\
> +	__ptr = !r ? (__map)->hva : NULL;					\
> +	r;									\
> +})
> +
>  static inline void kvm_vcpu_map_mark_dirty(struct kvm_vcpu *vcpu,
>  					   struct kvm_host_map *map)
>  {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 9093251beb39..e8d2e98b0068 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3114,9 +3114,10 @@ struct page *__gfn_to_page(struct kvm *kvm, gfn_t gfn, bool write)
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(__gfn_to_page);
>  
> -int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
> -		   bool writable)
> +int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, gpa_t len,
> +		   struct kvm_host_map *map, bool writable)
>  {
> +	gfn_t gfn = gpa_to_gfn(gpa);
>  	struct kvm_follow_pfn kfp = {
>  		.slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn),
>  		.gfn = gfn,
> @@ -3124,6 +3125,10 @@ int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
>  		.refcounted_page = &map->pinned_page,
>  		.pin = true,
>  	};
> +	kvm_pfn_t pfn;
> +
> +	if (WARN_ON_ONCE(offset_in_page(gpa) + len > PAGE_SIZE))
> +		return -EINVAL;

Maybe do the bounds checking after initializing 'map', then
kvm_vcpu_map_ptr() wouldn't need to explicitly set the pointer to NULL
on failure? There is already possibility of failure after initialization
anyway.

>  
>  	map->pinned_page = NULL;
>  	map->page = NULL;
> @@ -3131,20 +3136,25 @@ int __kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
>  	map->gfn = gfn;
>  	map->writable = writable;
>  
> -	map->pfn = kvm_follow_pfn(&kfp);
> -	if (is_error_noslot_pfn(map->pfn))
> +	pfn = kvm_follow_pfn(&kfp);
> +	if (is_error_noslot_pfn(pfn))
>  		return -EINVAL;
>  
> -	if (pfn_valid(map->pfn)) {
> -		map->page = pfn_to_page(map->pfn);
> +	map->hpa = pfn_to_hpa(pfn);
> +	if (pfn_valid(pfn)) {
> +		map->page = pfn_to_page(pfn);
>  		map->hva = kmap(map->page);
>  #ifdef CONFIG_HAS_IOMEM
>  	} else {
> -		map->hva = memremap(pfn_to_hpa(map->pfn), PAGE_SIZE, MEMREMAP_WB);
> +		map->hva = memremap(map->hpa, PAGE_SIZE, MEMREMAP_WB);
> +		if (!map->hva)
> +			return -EFAULT;
>  #endif
>  	}
>  
> -	return map->hva ? 0 : -EFAULT;
> +	map->hpa += offset_in_page(gpa);
> +	map->hva += offset_in_page(gpa);
> +	return 0;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(__kvm_vcpu_map);
[..]