Re: [PATCH v4 6/6] KVM: arm64: Add auto HDBSS enable/disable on dirty logging change

[Tian Zheng <zhengtian10@huawei.com>]

On 7/9/2026 7:34 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [Critical] Freeing HDBSS buffers synchronously in `kvm_arm_disable_hdbss_global()` without kicking vCPUs causes a hardware Use-After-Free and host memory corruption.
> - [High] The patch fails to enforce eager page table splitting when `KVM_DIRTY_LOG_INITIALLY_SET` is used, violating the HDBSS requirement.
> - [High] Global enable/disable functions mutate per-vCPU HDBSS state across all vCPUs without proper locking or KVM request mechanisms.
> - [High] Missing `kvm_unshare_hyp()` on the vCPU creation error path breaks pKVM memory ownership.
> - [High] Memslots are accessed without SRCU protection when flushing the HDBSS buffer.
> - [Medium] The dynamically calculated HDBSS buffer order is never saved, causing newly created vCPUs to allocate incorrect 1-page buffers.
> - [Medium] A conditional check in `kvm_arm_vcpu_destroy` leaks HDBSS buffers if a TOCTOU race left a vCPU with an allocated buffer while `enable_hdbss` is false.
> - [Medium] Missing immediate Context Synchronization Events (ISB) for control-plane system registers.
> --
> 
> --- Patch [6]: [PATCH v4 6/6] KVM: arm64: Add auto HDBSS enable/disable on dirty logging change ---
> commit c993a05c79f4338df277681c6534d32f65490e56
> Author: eillon <yezhenyu2@huawei.com>
> 
> KVM: arm64: Add auto HDBSS enable/disable on dirty logging change
> 
> This commit adds VM-level HDBSS enable/disable support tied to dirty logging
> changes. It automatically enables HDBSS when dirty logging starts and disables
> it when all memslots stop dirty logging.
> 
>> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
>> index 002366337637c..c5bf866c23ef2 100644
>> --- a/arch/arm64/kvm/dirty_bit.c
>> +++ b/arch/arm64/kvm/dirty_bit.c
> [ ... ]
>> +static int kvm_arm_enable_hdbss_global(struct kvm *kvm)
>> +{
>> +	int err;
>> +	unsigned long i;
>> +	unsigned int order;
>> +	struct kvm_vcpu *vcpu;
> [ ... ]
>> +	/* Turn it on */
>> +	order = hdbss_auto_select_order(kvm);
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		err = kvm_arm_vcpu_alloc_hdbss(vcpu, order);
> 
> [Severity: Medium]
> Will newly created vCPUs allocate buffers of the correct size?
> 
> The optimal HDBSS buffer order is calculated here based on memslot sizes and
> used to allocate buffers for currently online vCPUs, but the value doesn't
> seem to be saved to kvm->arch.hdbss_order.
> 
> When a new vCPU is created later, it appears kvm_arch_vcpu_create() allocates
> its buffer using the uninitialized kvm->arch.hdbss_order:
> 
> arch/arm64/kvm/arm.c:kvm_arch_vcpu_create() {
>      ...
>      err = kvm_arm_vcpu_alloc_hdbss(vcpu, vcpu->kvm->arch.hdbss_order);
>      ...
> }
> 
> Could this cause new vCPUs to receive order-0 buffers instead of the
> dynamically selected size, leading to mismatched sizes and excessive
> buffer-full exits?
> 

Addressed by redesign in v5. The dynamic order calculation is removed.
The buffer size is now stored in kvm->arch.hdbss_buffer_size (bytes),
set either by the user via KVM_CAP_ARM_HDBSS_BUFFER_SIZE (before vCPU
creation), or by kvm_arch_dirty_ring_size_updated() when dirty-ring is
configured, or defaults to PAGE_SIZE. Since it is stored in kvm->arch
before any vCPU exists, all vCPUs — including later-created ones —
allocate a consistent size.

> [Severity: High]
> Is it safe to directly mutate per-vCPU HDBSS state across all vCPUs from the
> thread handling the memslot ioctl?
> 
> This loop directly calls kvm_arm_vcpu_alloc_hdbss() to mutate vcpu->arch.hdbss
> without holding vcpu->mutex or using the KVM vCPU requests framework.
> 
> Could concurrently running vCPUs undergoing a context switch (like
> __load_hdbss) or fault path read partially updated, torn, or freed state,
> leading to unpredictable hardware register programming?
> 
>> +		if (err)
>> +			goto error_alloc;
>> +	}
> [ ... ]
>> +void kvm_arm_disable_hdbss_global(struct kvm *kvm)
>> +{
>> +	unsigned long i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	if (!kvm->arch.enable_hdbss) /* Already Off */
>> +		return;
>> +
>> +	/* Turn it off */
>> +	kvm->arch.mmu.vtcr &= ~(VTCR_EL2_HD | VTCR_EL2_HDBSS | VTCR_EL2_HA);
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		kvm_arm_vcpu_free_hdbss(vcpu);
> 

Addressed by redesign in v5. Enable/disable no longer touch any per-vCPU 
state. Buffers are pre-allocated at vCPU creation, and enable/disable 
only flips VTCR bits in kvm->arch.mmu.vtcr (a VM-level field written 
only from the memslot-ioctl path under slots_lock) followed by a kick. 
vCPUs pick up the new VTCR when __load_stage2() reprograms vtcr_el2 on 
re-entry.


> [Severity: Critical]
> Does this code risk hardware use-after-free and host memory corruption?
> 
> When dirty logging is disabled, the enable_hdbss flag and VTCR_EL2 bits are
> cleared, and the physical pages are immediately freed. Since this occurs under
> kvm->slots_lock, which does not prevent vCPUs from executing in guest mode,
> running vCPUs could continue executing with their hardware VTCR_EL2 and
> HDBSSBR_EL2 registers still active.
> 
> Should this function force running vCPUs to exit (e.g., via kvm_vcpu_kick() or
> a KVM request) before freeing the physical pages to prevent the hardware MMU
> from asynchronously writing dirty page records into newly freed memory?
> 
>> +
>> +	kvm->arch.enable_hdbss = false;
>> +}
> [ ... ]
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 949fb895add6a..484f48dae0003 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -2588,6 +2588,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>>   {
>>   	bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES;
> [ ... ]
>>   	if (log_dirty_pages) {
>>   
>>   		if (change == KVM_MR_DELETE)
>>   			return;
>>   
>>   		/*
>>   		 * Huge and normal pages are write-protected and split
>>   		 * on either of these two cases:
>>   		 *
>>   		 * 1. with initial-all-set: gradually with CLEAR ioctls,
>>   		 */
>>   		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
>>   			return;
> 

Valid point, addressed by redesign in v5. kvm_arm_disable_hdbss_global() 
no longer frees buffers at all — it only clears the VTCR HDBSS bits and 
kicks vCPUs out of guest mode via KVM_REQ_OUTSIDE_GUEST_MODE. Buffers 
are allocated at vCPU creation and freed only at vCPU destruction 
(PML-style), so there is no window where the MMU can write into freed 
memory.

> [Severity: High]
> Does this early return violate the HDBSS requirement for eager page table
> splitting?
> 
> The commit message states that enabling HDBSS requires disabling lazy split so
> that all block mappings are eagerly broken down before the buffer starts
> recording. However, if userspace utilizes KVM_DIRTY_LOG_INITIALLY_SET, this
> path returns early without calling kvm_mmu_split_memory_region().
> 
> Since HDBSS was already globally enabled earlier in this function, could it
> now operate on block mappings instead of page-granularity tables, defeating
> the hardware expectation and corrupting the dirty log state?
> 

Not an issue in the v5 design. With KVM_DIRTY_LOG_INITIALLY_SET, 
userspace clears the bitmap via CLEAR ioctls; the clear path 
write-protects, and v5's wrprotect strips DBM from blocks (->RO) so the 
first write faults and triggers the split, while level-3 pages go WC 
(DBM=1, S2AP_W=0) and are re-tracked by HDBSS. Since v5 supports lazy 
split in the HDBSS flow, the early return on INITIALLY_SET does not 
break dirty tracking.


> [Severity: Medium]
> Can a Time-of-Check to Time-of-Use (TOCTOU) race leak HDBSS buffers during
> vCPU destruction?
> 
> Looking at kvm_arm_vcpu_destroy() in arch/arm64/kvm/reset.c:
> 
> kvm_arm_vcpu_destroy() {
>      ...
>      if (vcpu->kvm->arch.enable_hdbss)
>          kvm_arm_vcpu_free_hdbss(vcpu);
>      ...
> }
> 
> If KVM_CREATE_VCPU allocates an HDBSS buffer but kvm_arm_disable_hdbss_global()
> runs before the vCPU is added to online_vcpus, the global disable loop would
> miss the vCPU and clear enable_hdbss. When the VM is destroyed, the condition
> would evaluate to false, skipping the cleanup.
> 
> Since kvm_arm_vcpu_free_hdbss() natively checks for the existence of the
> buffer, is the outer check against enable_hdbss necessary?
> 

Addressed in v5. kvm_arm_vcpu_destroy() calls kvm_arm_vcpu_free_hdbss() 
unconditionally (the function itself checks for buffer existence), and 
the enable_hdbss conditional — along with the flag itself — no longer 
exists.

> [Severity: High]
> Does the vCPU creation error path break pKVM memory ownership by missing a
> call to kvm_unshare_hyp()?
> 
> In arch/arm64/kvm/arm.c, kvm_arch_vcpu_create() shares the vCPU struct memory
> with the hypervisor. If the new allocation fails, the function cleans up and
> returns the error:
> 
> kvm_arch_vcpu_create() {
>      ...
>      if (vcpu->kvm->arch.enable_hdbss) {
>          err = kvm_arm_vcpu_alloc_hdbss(vcpu, vcpu->kvm->arch.hdbss_order);
>          if (err)
>              kvm_vgic_vcpu_destroy(vcpu);
>      }
> 
>      return err;
> }
> 
> Could the memory for the vcpu struct be subsequently freed back to the host
> allocator while still marked as shared with the hypervisor, violating pKVM
> trust boundaries and triggering a hypervisor panic upon reuse?
> 

Not applicable. HDBSS is VHE-only. The error path does not touch pKVM 
ownership — it simply cleans up the local allocation. pKVM support is 
explicitly out of scope and left as future work.

> [Severity: High]
> Does flushing the HDBSS buffer access memslots without SRCU protection?
> 
> kvm_flush_hdbss_buffer() processes dirty pages by resolving GFNs via
> kvm_vcpu_mark_page_dirty():
> 
> arch/arm64/kvm/dirty_bit.c:kvm_flush_hdbss_buffer() {
>      ...
>      kvm_vcpu_mark_page_dirty(vcpu, gpa >> PAGE_SHIFT);
>      ...
> }
> 
> This internal function requires srcu_read_lock(&kvm->srcu) to safely access
> the KVM memslots array. However, kvm_flush_hdbss_buffer() is invoked from
> kvm_arch_vcpu_put() and check_vcpu_requests(), neither of which hold the SRCU
> read lock.
> 
> Could this create a use-after-free risk when memslots are concurrently
> modified?
> 

Valid point, addressed in v5. kvm_flush_hdbss_buffer() now holds 
srcu_read_lock(&kvm->srcu) around the loop that resolves GFNs through 
the memslots, and releases it before resetting HDBSSPROD_EL2.

> [Severity: Medium]
> Are immediate Context Synchronization Events (ISB) missing for control-plane
> system registers?
> 
> The ARM64 subsystem rules require that every write to a control-plane system
> register must be followed by an isb() as the very next instruction.
> 
> In arch/arm64/kvm/dirty_bit.c, a memory store executes between the write and
> the isb():
> 
> kvm_flush_hdbss_buffer() {
>      ...
>      write_sysreg_s(0, SYS_HDBSSPROD_EL2);
>      vcpu->arch.hdbss.hdbssprod_el2 = 0;
>      isb();
>      ...
> }
> 
> Similarly, in arch/arm64/kvm/hyp/vhe/switch.c, two sysreg writes occur
> back-to-back without an intervening isb():
> 
> __load_hdbss() {
>      ...
>      write_sysreg_s(vcpu->arch.hdbss.hdbssbr_el2, SYS_HDBSSBR_EL2);
>      write_sysreg_s(vcpu->arch.hdbss.hdbssprod_el2, SYS_HDBSSPROD_EL2);
>      ...
> }
> 
> Could these patterns risk observing undefined pipeline state?
>


Duplicate of the ISB comments on patches 4/5.

Thanks!
Tian