* [PATCH 0/2] KVM: riscv: Unify and optimize FP register loops
@ 2026-10-26 3:41 Paul White
2026-10-26 3:41 ` [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers Paul White
2026-10-26 3:41 ` [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg " Paul White
0 siblings, 2 replies; 5+ messages in thread
From: Paul White @ 2026-10-26 3:41 UTC (permalink / raw)
To: anup, atish.patra, pjw, palmer, aou, alex
Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Paul White
The change to both patches in this series is a
structural/architectural one. Both functions set_reg and get_reg use
identical code to locate the register the application specified in the
token even though they do different things with that location
information: the set_reg function adds data the application requested
to send to that identified location (in the copy_from_user function)
while the get_reg function retrieves what data is at that identified
location and sends it to the application (using the copy_to_user
function). Everything else is the same. That is the only difference
between the code.
Therefore I was able to apply the same solution to both of the
function's finding-the-requested-register parts (which is everything
up until the final if statement) which were filled with redundancy.
The solution I came up with is to create local variables for the
entire function for all the variables of the original code that would
be different, depending on the floating point type. Then once I ask
whether it is a double floating point or single floating point mode, I
assign the aforementioned variables to the values they should have for
that mode so that when I go through the if statements that ask whether
the requested register number is a status control register or data
register, I only have to go through it once because the respective
floating point values/addresses would have been assigned to the
aforementioned local variables, so the whole code kind of changes or
is tailored to the floating point type.
This architectural change/solution will make the cpu execute the code
faster (as the cpu will have less decisions to make and the bare-metal
math used to figure out the address of the register is executed faster
by the cpu than looking up object properties and array indexes), is
more maintainable, and is more readable.
Paul White (2):
KVM: riscv: Eliminate redundancy in get_reg function via pointers
KVM: riscv: Eliminate redundancy in set_reg function via pointers
arch/riscv/kvm/vcpu_fp.c | 128 +++++++++++++++++++++++----------------
1 file changed, 76 insertions(+), 52 deletions(-)
--
2.53.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers
2026-10-26 3:41 [PATCH 0/2] KVM: riscv: Unify and optimize FP register loops Paul White
@ 2026-10-26 3:41 ` Paul White
2026-10-26 7:02 ` sashiko-bot
2026-10-26 3:41 ` [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg " Paul White
1 sibling, 1 reply; 5+ messages in thread
From: Paul White @ 2026-10-26 3:41 UTC (permalink / raw)
To: anup, atish.patra, pjw, palmer, aou, alex
Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Paul White
The original code is essentially asking: for each possible floating
point mode, ask if reg_num is a status control register or a data
register. It is redundant to ask those same questions for every
possible floating point mode case.
In order to only ask the "status control register or data register"
question once, the variables in the status control if statement and
the data register if statement that would be different depending on
the type of floating point mode (for example,
KVM_REG_RISCV_FP_F_REG(fcsr) would be KVM_REG_RISCV_FP_D_REG(fcsr) if
the floating point mode was double) were made local variables of the
entire function so that once the cpu passes the if statement inquiring
whether it is double point mode or single point mode, all the
variables that change depending on the type in the status control if
statements and data register if statements will be set to the
appropriate values they should be when in that mode before/when it
enters the status control and data register if statements, allowing me
to only have to ask those questions once and creating a dynamic code
that changes based on the information. This results in a "asking the
type then asking status control or data register question once"
structure rather than a type-questions, type-questions redundant
structure like before.
This results in faster cpu execution because despite the longer lines
of code the cpu has to make less decisions, which saves time in case,
due to its tendency to make assumptions about what an if statement will
evaluate to, it has to backpedal because it realized it needed to go
down a different if statement/path. Finding an address via my
bare-metal approach is also faster than looking up an object properties
and then an array. It is also more maintainable in the future. If the
original code needed to be changed, you would have to remember to do
the same thing to both type blocks whereas mine would just require a
change in the beginning if statements. If a new floating point mode was
added you have to ask those same questions all over again, which may
make the code longer in the long run.
I also renamed reg_num to token_reg_num in my code as I felt it made
it clearer that it was the register number the application wants and
requested in a token. It differentiated it from the many other
variables with the word reg in it.
Signed-off-by: Paul White <paul.white.kernel@gmail.com>
---
arch/riscv/kvm/vcpu_fp.c | 70 +++++++++++++++++++++++-----------------
1 file changed, 41 insertions(+), 29 deletions(-)
diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
index bb11e6757d34..16999a5c45de 100644
--- a/arch/riscv/kvm/vcpu_fp.c
+++ b/arch/riscv/kvm/vcpu_fp.c
@@ -83,41 +83,53 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
unsigned long __user *uaddr =
(unsigned long __user *)(unsigned long)reg->addr;
- unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
+ unsigned long token_reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
KVM_REG_SIZE_MASK |
rtype);
void *reg_val;
+ u32 *fcsr_addr;
+ void *f_array_addr;
+ size_t byte_offset = 0;
+ unsigned long offical_fcsr_num = 0;
+ unsigned long offical_first_reg = 0;
+ unsigned long offical_last_reg = 0;
+
if ((rtype == KVM_REG_RISCV_FP_F) &&
riscv_isa_extension_available(vcpu->arch.isa, F)) {
+ fcsr_addr = &cntx->fp.f.fcsr;
+ f_array_addr = &cntx->fp.f.f;
+ byte_offset = 4;
+ offical_fcsr_num = KVM_REG_RISCV_FP_F_REG(fcsr);
+ offical_first_reg = KVM_REG_RISCV_FP_F_REG(f[0]);
+ offical_last_reg = KVM_REG_RISCV_FP_F_REG(f[31]);
+ } else if ((rtype == KVM_REG_RISCV_FP_D) &&
+ riscv_isa_extension_available(vcpu->arch.isa, D)) {
+ fcsr_addr = &cntx->fp.d.fcsr;
+ f_array_addr = &cntx->fp.d.f;
+ byte_offset = 8;
+ offical_fcsr_num = KVM_REG_RISCV_FP_D_REG(fcsr);
+ offical_first_reg = KVM_REG_RISCV_FP_D_REG(f[0]);
+ offical_last_reg = KVM_REG_RISCV_FP_D_REG(f[31]);
+ } else {
+ return -ENOENT;
+ }
+
+ if (token_reg_num == offical_fcsr_num) {
if (KVM_REG_SIZE(reg->id) != sizeof(u32))
return -EINVAL;
- if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
- reg_val = &cntx->fp.f.fcsr;
- else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
- reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
- reg_num = array_index_nospec(reg_num,
- ARRAY_SIZE(cntx->fp.f.f));
- reg_val = &cntx->fp.f.f[reg_num];
- } else
- return -ENOENT;
- } else if ((rtype == KVM_REG_RISCV_FP_D) &&
- riscv_isa_extension_available(vcpu->arch.isa, D)) {
- if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
- if (KVM_REG_SIZE(reg->id) != sizeof(u32))
- return -EINVAL;
- reg_val = &cntx->fp.d.fcsr;
- } else if ((KVM_REG_RISCV_FP_D_REG(f[0]) <= reg_num) &&
- reg_num <= KVM_REG_RISCV_FP_D_REG(f[31])) {
- if (KVM_REG_SIZE(reg->id) != sizeof(u64))
- return -EINVAL;
- reg_num = array_index_nospec(reg_num,
- ARRAY_SIZE(cntx->fp.d.f));
- reg_val = &cntx->fp.d.f[reg_num];
- } else
- return -ENOENT;
- } else
+ reg_val = fcsr_addr;
+ } else if ((offical_first_reg <= token_reg_num) && (token_reg_num <= offical_last_reg)) {
+ if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
+ (KVM_REG_SIZE(reg->id) != sizeof(u64))) {
+ return -EINVAL;
+ }
+ token_reg_num = array_index_nospec(token_reg_num,
+ 32);
+ reg_val = (u8 *)f_array_addr + (token_reg_num * byte_offset);
+ } else {
return -ENOENT;
+ }
if (copy_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id)))
return -EFAULT;
@@ -144,9 +156,9 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
reg_val = &cntx->fp.f.fcsr;
else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
- reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
+ reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
reg_num = array_index_nospec(reg_num,
- ARRAY_SIZE(cntx->fp.f.f));
+ ARRAY_SIZE(cntx->fp.f.f));
reg_val = &cntx->fp.f.f[reg_num];
} else
return -ENOENT;
@@ -161,7 +173,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
if (KVM_REG_SIZE(reg->id) != sizeof(u64))
return -EINVAL;
reg_num = array_index_nospec(reg_num,
- ARRAY_SIZE(cntx->fp.d.f));
+ ARRAY_SIZE(cntx->fp.d.f));
reg_val = &cntx->fp.d.f[reg_num];
} else
return -ENOENT;
--
2.53.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg function via pointers
2026-10-26 3:41 [PATCH 0/2] KVM: riscv: Unify and optimize FP register loops Paul White
2026-10-26 3:41 ` [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers Paul White
@ 2026-10-26 3:41 ` Paul White
2026-10-26 7:02 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Paul White @ 2026-10-26 3:41 UTC (permalink / raw)
To: anup, atish.patra, pjw, palmer, aou, alex
Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Paul White
The original code is essentially asking: for each possible floating
point mode, ask if reg_num is a status control register or a data
register. It is redundant to ask those same questions for every
possible floating point mode case.
In order to only ask the "status control register or data register"
question once, the variables in the status control if statement and
the data register if statement that would be different depending on
the type of floating point mode (for example,
KVM_REG_RISCV_FP_F_REG(fcsr) would be KVM_REG_RISCV_FP_D_REG(fcsr) if
the floating point mode was double) were made local variables of the
entire function so that once the cpu passes the if statement inquiring
whether it is double point mode or single point mode, all the
variables that change depending on the type in the status control if
statements and data register if statements will be set to the
appropriate values they should be when in that mode before/when it
enters the status control and data register if statements, allowing me
to only have to ask those questions once and creating a dynamic code
that changes based on the information. This results in a "asking the
type then asking status control or data register question once"
structure rather than a type-questions, type-questions redundant
structure like before.
This results in faster cpu execution because despite the longer lines
of code the cpu has to make less decisions, which saves time in case,
due to its tendency to make assumptions about what an if statement will
evaluate to, it has to backpedal because it realized it needed to go
down a different if statement/path. Finding an address via my
bare-metal approach is also faster than looking up an object properties
and then an array. It is also more maintainable in the future. If the
original code needed to be changed, you would have to remember to do
the same thing to both type blocks whereas mine would just require a
change in the beginning if statements. If a new floating point mode was
added you have to ask those same questions all over again, which may
make the code longer in the long run.
I also renamed reg_num to token_reg_num in my code as I felt it made
it clearer that it was the register number the application wants and
requested in a token so that it can write data into that register. It
differentiated it from the many other variables with the word reg in
it.
Signed-off-by: Paul White <paul.white.kernel@gmail.com>
---
arch/riscv/kvm/vcpu_fp.c | 64 ++++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
index 16999a5c45de..fe4957603519 100644
--- a/arch/riscv/kvm/vcpu_fp.c
+++ b/arch/riscv/kvm/vcpu_fp.c
@@ -144,41 +144,53 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
unsigned long __user *uaddr =
(unsigned long __user *)(unsigned long)reg->addr;
- unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
+ unsigned long token_reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
KVM_REG_SIZE_MASK |
rtype);
void *reg_val;
+ u32 *fcsr_addr;
+ void *f_array_addr;
+ size_t byte_offset = 0;
+ unsigned long offical_fcsr_num = 0;
+ unsigned long offical_first_reg = 0;
+ unsigned long offical_last_reg = 0;
+
if ((rtype == KVM_REG_RISCV_FP_F) &&
riscv_isa_extension_available(vcpu->arch.isa, F)) {
+ fcsr_addr = &cntx->fp.f.fcsr;
+ f_array_addr = &cntx->fp.f.f;
+ byte_offset = 4;
+ offical_fcsr_num = KVM_REG_RISCV_FP_F_REG(fcsr);
+ offical_first_reg = KVM_REG_RISCV_FP_F_REG(f[0]);
+ offical_last_reg = KVM_REG_RISCV_FP_F_REG(f[31]);
+ } else if ((rtype == KVM_REG_RISCV_FP_D) &&
+ riscv_isa_extension_available(vcpu->arch.isa, D)) {
+ fcsr_addr = &cntx->fp.d.fcsr;
+ f_array_addr = &cntx->fp.d.f;
+ byte_offset = 8;
+ offical_fcsr_num = KVM_REG_RISCV_FP_D_REG(fcsr);
+ offical_first_reg = KVM_REG_RISCV_FP_D_REG(f[0]);
+ offical_last_reg = KVM_REG_RISCV_FP_D_REG(f[31]);
+ } else {
+ return -ENOENT;
+ }
+
+ if (token_reg_num == offical_fcsr_num) {
if (KVM_REG_SIZE(reg->id) != sizeof(u32))
return -EINVAL;
- if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
- reg_val = &cntx->fp.f.fcsr;
- else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
- reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
- reg_num = array_index_nospec(reg_num,
- ARRAY_SIZE(cntx->fp.f.f));
- reg_val = &cntx->fp.f.f[reg_num];
- } else
- return -ENOENT;
- } else if ((rtype == KVM_REG_RISCV_FP_D) &&
- riscv_isa_extension_available(vcpu->arch.isa, D)) {
- if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
- if (KVM_REG_SIZE(reg->id) != sizeof(u32))
- return -EINVAL;
- reg_val = &cntx->fp.d.fcsr;
- } else if ((KVM_REG_RISCV_FP_D_REG(f[0]) <= reg_num) &&
- reg_num <= KVM_REG_RISCV_FP_D_REG(f[31])) {
- if (KVM_REG_SIZE(reg->id) != sizeof(u64))
- return -EINVAL;
- reg_num = array_index_nospec(reg_num,
- ARRAY_SIZE(cntx->fp.d.f));
- reg_val = &cntx->fp.d.f[reg_num];
- } else
- return -ENOENT;
- } else
+ reg_val = fcsr_addr;
+ } else if ((offical_first_reg <= token_reg_num) && (token_reg_num <= offical_last_reg)) {
+ if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
+ (KVM_REG_SIZE(reg->id) != sizeof(u64))) {
+ return -EINVAL;
+ }
+ token_reg_num = array_index_nospec(token_reg_num,
+ 32);
+ reg_val = (u8 *)f_array_addr + (token_reg_num * byte_offset);
+ } else {
return -ENOENT;
+ }
if (copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id)))
return -EFAULT;
--
2.53.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg function via pointers
2026-10-26 3:41 ` [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg " Paul White
@ 2026-10-26 7:02 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-26 7:02 UTC (permalink / raw)
To: Paul White; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: riscv: wrong FP register size check in kvm_riscv_vcpu_set_reg_fp()
--
commit 22c4a5d5b5662dbf020a9f9d6d213a5477e96b0d
Author: Paul White <paul.white.kernel@gmail.com>
KVM: riscv: Eliminate redundancy in set_reg function via pointers
This commit refactors kvm_riscv_vcpu_set_reg_fp() to consolidate the register
type and bounds checks for single-precision and double-precision floating
point registers using local pointer and offset variables.
> diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> index 16999a5c45dec..fe49576035192 100644
> --- a/arch/riscv/kvm/vcpu_fp.c
> +++ b/arch/riscv/kvm/vcpu_fp.c
> @@ -144,41 +144,53 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> unsigned long __user *uaddr =
> (unsigned long __user *)(unsigned long)reg->addr;
> - unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> + unsigned long token_reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> KVM_REG_SIZE_MASK |
> rtype);
> void *reg_val;
>
> + u32 *fcsr_addr;
> + void *f_array_addr;
> + size_t byte_offset = 0;
> + unsigned long offical_fcsr_num = 0;
> + unsigned long offical_first_reg = 0;
> + unsigned long offical_last_reg = 0;
> +
> if ((rtype == KVM_REG_RISCV_FP_F) &&
> riscv_isa_extension_available(vcpu->arch.isa, F)) {
> + fcsr_addr = &cntx->fp.f.fcsr;
> + f_array_addr = &cntx->fp.f.f;
> + byte_offset = 4;
> + offical_fcsr_num = KVM_REG_RISCV_FP_F_REG(fcsr);
> + offical_first_reg = KVM_REG_RISCV_FP_F_REG(f[0]);
> + offical_last_reg = KVM_REG_RISCV_FP_F_REG(f[31]);
> + } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> + riscv_isa_extension_available(vcpu->arch.isa, D)) {
> + fcsr_addr = &cntx->fp.d.fcsr;
> + f_array_addr = &cntx->fp.d.f;
> + byte_offset = 8;
> + offical_fcsr_num = KVM_REG_RISCV_FP_D_REG(fcsr);
> + offical_first_reg = KVM_REG_RISCV_FP_D_REG(f[0]);
> + offical_last_reg = KVM_REG_RISCV_FP_D_REG(f[31]);
> + } else {
> + return -ENOENT;
> + }
> +
> + if (token_reg_num == offical_fcsr_num) {
> if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> return -EINVAL;
> - if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> - reg_val = &cntx->fp.f.fcsr;
> - else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
> - reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
> - reg_num = array_index_nospec(reg_num,
> - ARRAY_SIZE(cntx->fp.f.f));
> - reg_val = &cntx->fp.f.f[reg_num];
> - } else
> - return -ENOENT;
> - } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> - riscv_isa_extension_available(vcpu->arch.isa, D)) {
> - if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> - if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> - return -EINVAL;
> - reg_val = &cntx->fp.d.fcsr;
> - } else if ((KVM_REG_RISCV_FP_D_REG(f[0]) <= reg_num) &&
> - reg_num <= KVM_REG_RISCV_FP_D_REG(f[31])) {
> - if (KVM_REG_SIZE(reg->id) != sizeof(u64))
> - return -EINVAL;
> - reg_num = array_index_nospec(reg_num,
> - ARRAY_SIZE(cntx->fp.d.f));
> - reg_val = &cntx->fp.d.f[reg_num];
> - } else
> - return -ENOENT;
> - } else
> + reg_val = fcsr_addr;
> + } else if ((offical_first_reg <= token_reg_num) && (token_reg_num <= offical_last_reg)) {
> + if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
> + (KVM_REG_SIZE(reg->id) != sizeof(u64))) {
> + return -EINVAL;
> + }
> + token_reg_num = array_index_nospec(token_reg_num,
> + 32);
> + reg_val = (u8 *)f_array_addr + (token_reg_num * byte_offset);
> + } else {
> return -ENOENT;
> + }
>
> if (copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id)))
> return -EFAULT;
[Severity: High]
Can this size check allow an out-of-bounds write to cntx->fp.f.f[] when rtype
is KVM_REG_RISCV_FP_F and userspace passes KVM_REG_SIZE_U64 in reg->id via
KVM_SET_ONE_REG?
Previously, kvm_riscv_vcpu_set_reg_fp() required KVM_REG_SIZE(reg->id) ==
sizeof(u32) for KVM_REG_RISCV_FP_F registers and KVM_REG_SIZE(reg->id) ==
sizeof(u64) for KVM_REG_RISCV_FP_D data registers f[0]..f[31].
With the combined check:
if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
(KVM_REG_SIZE(reg->id) != sizeof(u64))) {
return -EINVAL;
}
an 8-byte size is accepted when rtype is KVM_REG_RISCV_FP_F (where
f_array_addr points to cntx->fp.f.f and byte_offset is 4). The subsequent
copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id)) then writes 8 bytes
into a 4-byte element of cntx->fp.f.f[32], overwriting the adjacent register
cntx->fp.f.f[token_reg_num + 1] or overflowing cntx->fp.f.f[] into
cntx->fp.f.fcsr when token_reg_num is 31.
Conversely, when rtype is KVM_REG_RISCV_FP_D (where byte_offset is 8), a
4-byte size is now accepted for f[0]..f[31] instead of returning -EINVAL,
resulting in a partial 4-byte write to cntx->fp.d.f[token_reg_num].
Note that the same check was also introduced in kvm_riscv_vcpu_get_reg_fp()
earlier in this series, where copy_to_user() can similarly read 8 bytes from
a 4-byte element of cntx->fp.f.f[32].
Should both functions check KVM_REG_SIZE(reg->id) != byte_offset for the data
registers instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792982932.git.paul.white.kernel@gmail.com?part=2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers
2026-10-26 3:41 ` [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers Paul White
@ 2026-10-26 7:02 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-26 7:02 UTC (permalink / raw)
To: Paul White; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: riscv: mismatched register size check in kvm_riscv_vcpu_get_reg_fp()
--
commit 0e99059ef6014093a2535ab4ecda318fe5ee3489
Author: Paul White <paul.white.kernel@gmail.com>
KVM: riscv: Eliminate redundancy in get_reg function via pointers
This commit refactors kvm_riscv_vcpu_get_reg_fp() to consolidate the
register lookup and size checks for single-precision and double-precision
floating-point registers using mode-dependent local variables and pointer
offsets.
> diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> index bb11e6757d349..16999a5c45dec 100644
> --- a/arch/riscv/kvm/vcpu_fp.c
> +++ b/arch/riscv/kvm/vcpu_fp.c
> @@ -83,41 +83,53 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> unsigned long __user *uaddr =
> (unsigned long __user *)(unsigned long)reg->addr;
> - unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> + unsigned long token_reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> KVM_REG_SIZE_MASK |
> rtype);
> void *reg_val;
>
> + u32 *fcsr_addr;
> + void *f_array_addr;
> + size_t byte_offset = 0;
> + unsigned long offical_fcsr_num = 0;
> + unsigned long offical_first_reg = 0;
> + unsigned long offical_last_reg = 0;
> +
> if ((rtype == KVM_REG_RISCV_FP_F) &&
> riscv_isa_extension_available(vcpu->arch.isa, F)) {
> + fcsr_addr = &cntx->fp.f.fcsr;
> + f_array_addr = &cntx->fp.f.f;
> + byte_offset = 4;
> + offical_fcsr_num = KVM_REG_RISCV_FP_F_REG(fcsr);
> + offical_first_reg = KVM_REG_RISCV_FP_F_REG(f[0]);
> + offical_last_reg = KVM_REG_RISCV_FP_F_REG(f[31]);
> + } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> + riscv_isa_extension_available(vcpu->arch.isa, D)) {
> + fcsr_addr = &cntx->fp.d.fcsr;
> + f_array_addr = &cntx->fp.d.f;
> + byte_offset = 8;
> + offical_fcsr_num = KVM_REG_RISCV_FP_D_REG(fcsr);
> + offical_first_reg = KVM_REG_RISCV_FP_D_REG(f[0]);
> + offical_last_reg = KVM_REG_RISCV_FP_D_REG(f[31]);
> + } else {
> + return -ENOENT;
> + }
> +
> + if (token_reg_num == offical_fcsr_num) {
> if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> return -EINVAL;
> - if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> - reg_val = &cntx->fp.f.fcsr;
> - else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
> - reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
> - reg_num = array_index_nospec(reg_num,
> - ARRAY_SIZE(cntx->fp.f.f));
> - reg_val = &cntx->fp.f.f[reg_num];
> - } else
> - return -ENOENT;
> - } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> - riscv_isa_extension_available(vcpu->arch.isa, D)) {
> - if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> - if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> - return -EINVAL;
> - reg_val = &cntx->fp.d.fcsr;
> - } else if ((KVM_REG_RISCV_FP_D_REG(f[0]) <= reg_num) &&
> - reg_num <= KVM_REG_RISCV_FP_D_REG(f[31])) {
> - if (KVM_REG_SIZE(reg->id) != sizeof(u64))
> - return -EINVAL;
> - reg_num = array_index_nospec(reg_num,
> - ARRAY_SIZE(cntx->fp.d.f));
> - reg_val = &cntx->fp.d.f[reg_num];
> - } else
> - return -ENOENT;
> - } else
> + reg_val = fcsr_addr;
> + } else if ((offical_first_reg <= token_reg_num) && (token_reg_num <= offical_last_reg)) {
> + if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
> + (KVM_REG_SIZE(reg->id) != sizeof(u64))) {
> + return -EINVAL;
> + }
> + token_reg_num = array_index_nospec(token_reg_num,
> + 32);
> + reg_val = (u8 *)f_array_addr + (token_reg_num * byte_offset);
> + } else {
> return -ENOENT;
> + }
>
> if (copy_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id)))
> return -EFAULT;
[Severity: High]
Can this read past the bounds of an element in cntx->fp.f.f[] when userspace
requests a single-precision register with a 64-bit size?
In kvm_riscv_vcpu_get_reg_fp(), the merged size check for floating-point data
registers allows both sizeof(u32) and sizeof(u64) regardless of whether rtype
is KVM_REG_RISCV_FP_F (where byte_offset is 4 and f_array_addr points to the
32-bit array cntx->fp.f.f) or KVM_REG_RISCV_FP_D (where byte_offset is 8 and
f_array_addr points to the 64-bit array cntx->fp.d.f).
If userspace invokes the vcpu ioctl path:
kvm_arch_vcpu_ioctl() ->
kvm_riscv_vcpu_get_reg() ->
kvm_riscv_vcpu_get_reg_fp()
with rtype == KVM_REG_RISCV_FP_F and KVM_REG_SIZE(reg->id) == sizeof(u64),
the size check now passes instead of returning -EINVAL. Then copy_to_user()
copies 8 bytes from a 4-byte slot at (u8 *)f_array_addr + (token_reg_num * 4),
reading into the adjacent element of cntx->fp.f.f[] (or past cntx->fp.f.f[31]
into cntx->fp.f.fcsr). Conversely, requesting a KVM_REG_RISCV_FP_D data
register with sizeof(u32) also passes and copies only 4 bytes instead of
returning -EINVAL.
Should the size check here compare against byte_offset instead:
if (KVM_REG_SIZE(reg->id) != byte_offset)
return -EINVAL;
so that the register size must match the width of the selected floating-point
mode?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792982932.git.paul.white.kernel@gmail.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-10-26 7:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-26 3:41 [PATCH 0/2] KVM: riscv: Unify and optimize FP register loops Paul White
2026-10-26 3:41 ` [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers Paul White
2026-10-26 7:02 ` sashiko-bot
2026-10-26 3:41 ` [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg " Paul White
2026-10-26 7:02 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox