diff mbox series

RISC-V: KVM: Fix shift-out-of-bounds in make_xfence_request()

Message ID 20260403061302.2203179-1-xujiakai2025@iscas.ac.cn
State Superseded
Headers show
Series RISC-V: KVM: Fix shift-out-of-bounds in make_xfence_request() | expand

Commit Message

Jiakai Xu April 3, 2026, 6:13 a.m. UTC 
The make_xfence_request() function uses a shift operation to check if a
vCPU is in the hart mask:

  if (!(hmask & (1UL << (vcpu->vcpu_id - hbase))))

However, when the difference between vcpu_id and hbase
is >= BITS_PER_LONG, the shift operation causes undefined behavior.

This was detected by UBSAN:
  UBSAN: shift-out-of-bounds in arch/riscv/kvm/tlb.c:343:23
  shift exponent 256 is too large for 64-bit type 'long unsigned int'

Fix this by adding a bounds check before the shift operation.

This bug was found by fuzzing the KVM RISC-V interface.

Fixes: 13acfec2dbcc ("RISC-V: KVM: Add remote HFENCE functions based on VCPU requests")
Signed-off-by: Jiakai Xu <jiakaiPeanut@gmail.com>
Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
---
 arch/riscv/kvm/tlb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Andrew Jones April 3, 2026, 3:30 p.m. UTC | #1 
On Fri, Apr 03, 2026 at 06:13:02AM +0000, Jiakai Xu wrote:
> The make_xfence_request() function uses a shift operation to check if a
> vCPU is in the hart mask:
> 
>   if (!(hmask & (1UL << (vcpu->vcpu_id - hbase))))
> 
> However, when the difference between vcpu_id and hbase
> is >= BITS_PER_LONG, the shift operation causes undefined behavior.
> 
> This was detected by UBSAN:
>   UBSAN: shift-out-of-bounds in arch/riscv/kvm/tlb.c:343:23
>   shift exponent 256 is too large for 64-bit type 'long unsigned int'
> 
> Fix this by adding a bounds check before the shift operation.
> 
> This bug was found by fuzzing the KVM RISC-V interface.
> 
> Fixes: 13acfec2dbcc ("RISC-V: KVM: Add remote HFENCE functions based on VCPU requests")
> Signed-off-by: Jiakai Xu <jiakaiPeanut@gmail.com>
> Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
> ---
>  arch/riscv/kvm/tlb.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
> index ff1aeac4eb8eb..500e001513a11 100644
> --- a/arch/riscv/kvm/tlb.c
> +++ b/arch/riscv/kvm/tlb.c
> @@ -333,14 +333,17 @@ static void make_xfence_request(struct kvm *kvm,
>  	unsigned long i;
>  	struct kvm_vcpu *vcpu;
>  	unsigned int actual_req = req;
> +	unsigned int idx;
>  	DECLARE_BITMAP(vcpu_mask, KVM_MAX_VCPUS);
>  
>  	bitmap_zero(vcpu_mask, KVM_MAX_VCPUS);
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		if (hbase != -1UL) {
> -			if (vcpu->vcpu_id < hbase)
> +			idx = vcpu->vcpu_id - hbase;
> +
> +			if (idx < 0 || idx >= BITS_PER_LONG)

idx is unsigned, so I would expect this 'idx < 0' to produce a warning
that it's always false.

I wouldn't introduce idx and just change the current condition instead

  if (vcpu->vcpu_id < hbase || vcpu->vcpu_id >= hbase + BITS_PER_LONG)
     continue;

>  				continue;
> -			if (!(hmask & (1UL << (vcpu->vcpu_id - hbase))))
> +			if (!(hmask & (1UL << idx)))
>  				continue;
>  		}
>  
> -- 
> 2.34.1
> 

unsigned arithmetic makes my head hurt. It would hurt less if we
guaranteed the ranges of all the components. vcpu_ids are
guaranteed to be [ 0, min(KVM_MAX_VCPU_IDS, INT_MAX) ), see
kvm_vm_ioctl_create_vcpu(). We should also test and reject hbase
and hmask in all the SBI calls to ensure that hbase is in the
same range as vcpu_ids and hmask is in the [0, BITS_PER_LONG)
range.

Thanks,
drew
Andrew Jones April 3, 2026, 3:38 p.m. UTC | #2 
On Fri, Apr 03, 2026 at 10:30:35AM -0500, Andrew Jones wrote:
> On Fri, Apr 03, 2026 at 06:13:02AM +0000, Jiakai Xu wrote:
> > The make_xfence_request() function uses a shift operation to check if a
> > vCPU is in the hart mask:
> > 
> >   if (!(hmask & (1UL << (vcpu->vcpu_id - hbase))))
> > 
> > However, when the difference between vcpu_id and hbase
> > is >= BITS_PER_LONG, the shift operation causes undefined behavior.
> > 
> > This was detected by UBSAN:
> >   UBSAN: shift-out-of-bounds in arch/riscv/kvm/tlb.c:343:23
> >   shift exponent 256 is too large for 64-bit type 'long unsigned int'
> > 
> > Fix this by adding a bounds check before the shift operation.
> > 
> > This bug was found by fuzzing the KVM RISC-V interface.
> > 
> > Fixes: 13acfec2dbcc ("RISC-V: KVM: Add remote HFENCE functions based on VCPU requests")
> > Signed-off-by: Jiakai Xu <jiakaiPeanut@gmail.com>
> > Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
> > ---
> >  arch/riscv/kvm/tlb.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
> > index ff1aeac4eb8eb..500e001513a11 100644
> > --- a/arch/riscv/kvm/tlb.c
> > +++ b/arch/riscv/kvm/tlb.c
> > @@ -333,14 +333,17 @@ static void make_xfence_request(struct kvm *kvm,
> >  	unsigned long i;
> >  	struct kvm_vcpu *vcpu;
> >  	unsigned int actual_req = req;
> > +	unsigned int idx;
> >  	DECLARE_BITMAP(vcpu_mask, KVM_MAX_VCPUS);
> >  
> >  	bitmap_zero(vcpu_mask, KVM_MAX_VCPUS);
> >  	kvm_for_each_vcpu(i, vcpu, kvm) {
> >  		if (hbase != -1UL) {
> > -			if (vcpu->vcpu_id < hbase)
> > +			idx = vcpu->vcpu_id - hbase;
> > +
> > +			if (idx < 0 || idx >= BITS_PER_LONG)
> 
> idx is unsigned, so I would expect this 'idx < 0' to produce a warning
> that it's always false.
> 
> I wouldn't introduce idx and just change the current condition instead
> 
>   if (vcpu->vcpu_id < hbase || vcpu->vcpu_id >= hbase + BITS_PER_LONG)
>      continue;
> 
> >  				continue;
> > -			if (!(hmask & (1UL << (vcpu->vcpu_id - hbase))))
> > +			if (!(hmask & (1UL << idx)))
> >  				continue;
> >  		}
> >  
> > -- 
> > 2.34.1
> > 
> 
> unsigned arithmetic makes my head hurt. It would hurt less if we
> guaranteed the ranges of all the components. vcpu_ids are
> guaranteed to be [ 0, min(KVM_MAX_VCPU_IDS, INT_MAX) ), see
> kvm_vm_ioctl_create_vcpu(). We should also test and reject hbase
> and hmask in all the SBI calls to ensure that hbase is in the
> same range as vcpu_ids and hmask is in the [0, BITS_PER_LONG)
> range.

eh, scratch this last phrase. Of course hmask can be [0, ULONG_MAX], not
just [0, BITS_PER_LONG) - so it doesn't need a range check. I guess my
head hurt too much when I wrote that...

drew
Jiakai Xu April 3, 2026, 11:07 p.m. UTC | #3 
> idx is unsigned, so I would expect this 'idx < 0' to produce a warning
> that it's always false.
> 
> I wouldn't introduce idx and just change the current condition instead
> 
>   if (vcpu->vcpu_id < hbase || vcpu->vcpu_id >= hbase + BITS_PER_LONG)
>      continue;

Thanks for the review!

You're right, the 'idx < 0' check is redundant since idx is unsigned.
Will drop the idx variable and use your suggested condition directly.

Will send a v2 shortly.

Thanks,
Jiakai
diff mbox series

Patch

diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
index ff1aeac4eb8eb..500e001513a11 100644
--- a/arch/riscv/kvm/tlb.c
+++ b/arch/riscv/kvm/tlb.c
@@ -333,14 +333,17 @@  static void make_xfence_request(struct kvm *kvm,
 	unsigned long i;
 	struct kvm_vcpu *vcpu;
 	unsigned int actual_req = req;
+	unsigned int idx;
 	DECLARE_BITMAP(vcpu_mask, KVM_MAX_VCPUS);
 
 	bitmap_zero(vcpu_mask, KVM_MAX_VCPUS);
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		if (hbase != -1UL) {
-			if (vcpu->vcpu_id < hbase)
+			idx = vcpu->vcpu_id - hbase;
+
+			if (idx < 0 || idx >= BITS_PER_LONG)
 				continue;
-			if (!(hmask & (1UL << (vcpu->vcpu_id - hbase))))
+			if (!(hmask & (1UL << idx)))
 				continue;
 		}