Re: [PATCH 10/24] KVM: Include memory protections in result of gfn->hva conversion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference in gfn_to_hva_prot() and kvm_vcpu_gfn_to_hva_prot() when applying memory attributes.
--

--- Patch [10]: [PATCH 10/24] KVM: Include memory protections in result of gfn->hva conversion ---
commit bca8d72704eb8c45ae205232d29cab272cbba365
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: Include memory protections in result of gfn->hva conversion

This commit adds memory attribute checks to paths leading to guest memory
accesses. Functions like gfn_to_hva and kvm_vcpu_gfn_to_hva now return error
values based on memory attributes, though cached variants will need separate
changes.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -2796,15 +2821,37 @@ unsigned long gfn_to_hva_memslot_prot(struct kvm_memory_slot *slot,
>  unsigned long gfn_to_hva_prot(struct kvm *kvm, gfn_t gfn, bool *writable)
>  {
>  	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
> +	unsigned long addr;
> +
> +	addr = gfn_to_hva_memslot_prot(slot, gfn, writable);
> +	if (kvm_is_error_hva(addr))
> +		return addr;
> +
> +	if (!kvm_mem_attributes_may_read_gfn(kvm, gfn))
> +		return KVM_HVA_ERR_BAD;
>  
> -	return gfn_to_hva_memslot_prot(slot, gfn, writable);
> +	if (!kvm_mem_attributes_may_write_gfn(kvm, gfn))
> +		*writable = false;

[Severity: High]
Does this code unconditionally dereference the writable pointer?

If a caller invokes gfn_to_hva_prot() with a NULL writable pointer, and the
GFN has memory attributes that restrict write access, will this crash the
kernel? The KVM API allows the writable pointer to be NULL.

> +
> +	return addr;
>  }
>  
>  unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable)
>  {
>  	struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
> +	unsigned long addr;
> +
> +	addr = gfn_to_hva_memslot_prot(slot, gfn, writable);
> +	if (kvm_is_error_hva(addr))
> +		return addr;
> +
> +	if (!kvm_mem_attributes_may_read_gfn(vcpu->kvm, gfn))
> +		return KVM_HVA_ERR_BAD;
> +
> +	if (!kvm_mem_attributes_may_write_gfn(vcpu->kvm, gfn))
> +		*writable = false;

[Severity: High]
Similar to the above, can this cause a NULL pointer dereference if
kvm_vcpu_gfn_to_hva_prot() is called with a NULL writable pointer?

>  
> -	return gfn_to_hva_memslot_prot(slot, gfn, writable);
> +	return addr;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716181456.402786-1-pbonzini@redhat.com?part=10