KVM functions use "long" return values for functions that are wired up
to "struct file_operations", but otherwise use "int" return values for
functions that can return 0/-errno in order to avoid unintentional
divergences between 32-bit and 64-bit kernels. Some related functions
that are not part of a "struct file_operations" still use "long", though,
which can cause confusion or even subtle problems (see the x86 and the
arm64 patches). Thus let's standardize on using "int" for return values
in these functions to avoid such problems in the future.
v2:
- Remove KVM_GET_NR_MMU_PAGES ioctl instead of trying to work-around
the problem (the ioctl is not used in any userspace app anyway)
- Add check for INT_MAX to the arm64 patch
- Added Reviewed-bys from v1
Thomas Huth (6):
KVM: PPC: Standardize on "int" return types in the powerpc KVM code
KVM: s390: Use "int" as return type for kvm_s390_get/set_skeys()
KVM: x86: Remove the KVM_GET_NR_MMU_PAGES ioctl
KVM: arm64: Limit length in kvm_vm_ioctl_mte_copy_tags() to INT_MAX
KVM: Standardize on "int" return types instead of "long" in kvm_main.c
KVM: Change return type of kvm_arch_vm_ioctl() to "int"
Documentation/virt/kvm/api.rst | 3 ++-
arch/arm64/include/asm/kvm_host.h | 4 ++--
arch/arm64/kvm/arm.c | 3 +--
arch/arm64/kvm/guest.c | 8 ++++++--
arch/mips/kvm/mips.c | 4 ++--
arch/powerpc/include/asm/kvm_ppc.h | 14 +++++++-------
arch/powerpc/kvm/book3s_64_mmu_hv.c | 14 +++++++-------
arch/powerpc/kvm/book3s_64_vio.c | 4 ++--
arch/powerpc/kvm/book3s_hv.c | 6 +++---
arch/powerpc/kvm/book3s_pr.c | 4 ++--
arch/powerpc/kvm/powerpc.c | 5 ++---
arch/riscv/kvm/vm.c | 3 +--
arch/s390/kvm/kvm-s390.c | 7 +++----
arch/x86/kvm/x86.c | 11 +----------
include/linux/kvm_host.h | 3 +--
include/uapi/linux/kvm.h | 2 +-
tools/include/uapi/linux/kvm.h | 2 +-
virt/kvm/kvm_main.c | 4 ++--
18 files changed, 46 insertions(+), 55 deletions(-)
--
2.31.1
These two functions only return normal integers, so it does not
make sense to declare the return type as "long" here.
Reviewed-by: Claudio Imbrenda <[email protected]>
Signed-off-by: Thomas Huth <[email protected]>
---
arch/s390/kvm/kvm-s390.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e4890e04b210..8ad1972b8a73 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1992,7 +1992,7 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
return ret;
}
-static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
+static int kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
{
uint8_t *keys;
uint64_t hva;
@@ -2040,7 +2040,7 @@ static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
return r;
}
-static long kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
+static int kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
{
uint8_t *keys;
uint64_t hva;
--
2.31.1
In case of success, this function returns the amount of handled bytes.
However, this does not work for large values: The function is called
from kvm_arch_vm_ioctl() (which still returns a long), which in turn
is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that function
stores the return value in an "int r" variable. So the upper 32-bits
of the "long" return value are lost there.
KVM ioctl functions should only return "int" values, so let's limit
the amount of bytes that can be requested here to INT_MAX to avoid
the problem with the truncated return value. We can then also change
the return type of the function to "int" to make it clearer that it
is not possible to return a "long" here.
Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest")
Signed-off-by: Thomas Huth <[email protected]>
---
Documentation/virt/kvm/api.rst | 3 ++-
arch/arm64/include/asm/kvm_host.h | 4 ++--
arch/arm64/kvm/guest.c | 8 ++++++--
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 0a67cb738013..f184427931fa 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5553,7 +5553,8 @@ with the KVM_XEN_VCPU_GET_ATTR ioctl.
};
Copies Memory Tagging Extension (MTE) tags to/from guest tag memory. The
-``guest_ipa`` and ``length`` fields must be ``PAGE_SIZE`` aligned. The ``addr``
+``guest_ipa`` and ``length`` fields must be ``PAGE_SIZE`` aligned.
+``length`` must not be bigger than 2^31 - PAGE_SIZE bytes. The ``addr``
field must point to a buffer which the tags will be copied to or from.
``flags`` specifies the direction of copy, either ``KVM_ARM_TAGS_TO_GUEST`` or
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 35a159d131b5..b1a16343767f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
struct kvm_device_attr *attr);
-long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
- struct kvm_arm_copy_mte_tags *copy_tags);
+int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
+ struct kvm_arm_copy_mte_tags *copy_tags);
/* Guest/host FPSIMD coordination helpers */
int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index cf4c495a4321..cadef953046f 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
return ret;
}
-long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
- struct kvm_arm_copy_mte_tags *copy_tags)
+int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
+ struct kvm_arm_copy_mte_tags *copy_tags)
{
gpa_t guest_ipa = copy_tags->guest_ipa;
size_t length = copy_tags->length;
@@ -1035,6 +1035,10 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK)
return -EINVAL;
+ /* Lengths above INT_MAX cannot be represented in the return value */
+ if (length > INT_MAX)
+ return -EINVAL;
+
gfn = gpa_to_gfn(guest_ipa);
mutex_lock(&kvm->slots_lock);
--
2.31.1
The KVM_GET_NR_MMU_PAGES ioctl is quite questionable on 64-bit hosts
since it fails to return the full 64 bits of the value that can be
set with the corresponding KVM_SET_NR_MMU_PAGES call. Its "long" return
value is truncated into an "int" in the kvm_arch_vm_ioctl() function.
Since this ioctl also never has been used by userspace applications
(QEMU, Google's internal VMM, kvmtool and CrosVM have been checked),
it's likely the best if we remove this badly designed ioctl before
anybody really tries to use it.
Signed-off-by: Thomas Huth <[email protected]>
---
arch/x86/kvm/x86.c | 8 --------
include/uapi/linux/kvm.h | 2 +-
tools/include/uapi/linux/kvm.h | 2 +-
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index da4bbd043a7b..2d210ab47e21 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6007,11 +6007,6 @@ static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
return 0;
}
-static unsigned long kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
-{
- return kvm->arch.n_max_mmu_pages;
-}
-
static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
{
struct kvm_pic *pic = kvm->arch.vpic;
@@ -6689,9 +6684,6 @@ long kvm_arch_vm_ioctl(struct file *filp,
case KVM_SET_NR_MMU_PAGES:
r = kvm_vm_ioctl_set_nr_mmu_pages(kvm, arg);
break;
- case KVM_GET_NR_MMU_PAGES:
- r = kvm_vm_ioctl_get_nr_mmu_pages(kvm);
- break;
case KVM_CREATE_IRQCHIP: {
mutex_lock(&kvm->lock);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 55155e262646..59dc7e5d4974 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1442,7 +1442,7 @@ struct kvm_vfio_spapr_tce {
#define KVM_CREATE_VCPU _IO(KVMIO, 0x41)
#define KVM_GET_DIRTY_LOG _IOW(KVMIO, 0x42, struct kvm_dirty_log)
#define KVM_SET_NR_MMU_PAGES _IO(KVMIO, 0x44)
-#define KVM_GET_NR_MMU_PAGES _IO(KVMIO, 0x45)
+#define KVM_GET_NR_MMU_PAGES _IO(KVMIO, 0x45) /* deprecated */
#define KVM_SET_USER_MEMORY_REGION _IOW(KVMIO, 0x46, \
struct kvm_userspace_memory_region)
#define KVM_SET_TSS_ADDR _IO(KVMIO, 0x47)
diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
index 55155e262646..59dc7e5d4974 100644
--- a/tools/include/uapi/linux/kvm.h
+++ b/tools/include/uapi/linux/kvm.h
@@ -1442,7 +1442,7 @@ struct kvm_vfio_spapr_tce {
#define KVM_CREATE_VCPU _IO(KVMIO, 0x41)
#define KVM_GET_DIRTY_LOG _IOW(KVMIO, 0x42, struct kvm_dirty_log)
#define KVM_SET_NR_MMU_PAGES _IO(KVMIO, 0x44)
-#define KVM_GET_NR_MMU_PAGES _IO(KVMIO, 0x45)
+#define KVM_GET_NR_MMU_PAGES _IO(KVMIO, 0x45) /* deprecated */
#define KVM_SET_USER_MEMORY_REGION _IOW(KVMIO, 0x46, \
struct kvm_userspace_memory_region)
#define KVM_SET_TSS_ADDR _IO(KVMIO, 0x47)
--
2.31.1
All kvm_arch_vm_ioctl() implementations now only deal with "int"
types as return values, so we can change the return type of these
functions to use "int" instead of "long".
Signed-off-by: Thomas Huth <[email protected]>
---
arch/arm64/kvm/arm.c | 3 +--
arch/mips/kvm/mips.c | 4 ++--
arch/powerpc/kvm/powerpc.c | 5 ++---
arch/riscv/kvm/vm.c | 3 +--
arch/s390/kvm/kvm-s390.c | 3 +--
arch/x86/kvm/x86.c | 3 +--
include/linux/kvm_host.h | 3 +--
7 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 9c5573bc4614..e791ad6137b8 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1449,8 +1449,7 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
}
}
-long kvm_arch_vm_ioctl(struct file *filp,
- unsigned int ioctl, unsigned long arg)
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
{
struct kvm *kvm = filp->private_data;
void __user *argp = (void __user *)arg;
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index a25e0b73ee70..84cadaa2c2d3 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -1003,9 +1003,9 @@ void kvm_arch_flush_remote_tlbs_memslot(struct kvm *kvm,
kvm_flush_remote_tlbs(kvm);
}
-long kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
{
- long r;
+ int r;
switch (ioctl) {
default:
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 04494a4fb37a..6f6ba55c224f 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -2386,12 +2386,11 @@ static int kvmppc_get_cpu_char(struct kvm_ppc_cpu_char *cp)
}
#endif
-long kvm_arch_vm_ioctl(struct file *filp,
- unsigned int ioctl, unsigned long arg)
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
{
struct kvm *kvm __maybe_unused = filp->private_data;
void __user *argp = (void __user *)arg;
- long r;
+ int r;
switch (ioctl) {
case KVM_PPC_GET_PVINFO: {
diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
index 65a964d7e70d..c13130ab459a 100644
--- a/arch/riscv/kvm/vm.c
+++ b/arch/riscv/kvm/vm.c
@@ -87,8 +87,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
return r;
}
-long kvm_arch_vm_ioctl(struct file *filp,
- unsigned int ioctl, unsigned long arg)
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
{
return -EINVAL;
}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8ad1972b8a73..86ca49814983 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2850,8 +2850,7 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
return r;
}
-long kvm_arch_vm_ioctl(struct file *filp,
- unsigned int ioctl, unsigned long arg)
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
{
struct kvm *kvm = filp->private_data;
void __user *argp = (void __user *)arg;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2d210ab47e21..52a8c993cd55 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6645,8 +6645,7 @@ static int kvm_vm_ioctl_set_clock(struct kvm *kvm, void __user *argp)
return 0;
}
-long kvm_arch_vm_ioctl(struct file *filp,
- unsigned int ioctl, unsigned long arg)
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
{
struct kvm *kvm = filp->private_data;
void __user *argp = (void __user *)arg;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4f26b244f6d0..ed2f1f02976b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1398,8 +1398,7 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
bool line_status);
int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
struct kvm_enable_cap *cap);
-long kvm_arch_vm_ioctl(struct file *filp,
- unsigned int ioctl, unsigned long arg);
+int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg);
long kvm_arch_vm_compat_ioctl(struct file *filp, unsigned int ioctl,
unsigned long arg);
--
2.31.1
KVM functions use "long" return values for functions that are wired up
to "struct file_operations", but otherwise use "int" return values for
functions that can return 0/-errno in order to avoid unintentional
divergences between 32-bit and 64-bit kernels.
Some code still uses "long" in unnecessary spots, though, which can
cause a little bit of confusion and unnecessary size casts. Let's
change these spots to use "int" types, too.
Signed-off-by: Thomas Huth <[email protected]>
---
virt/kvm/kvm_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9c60384b5ae0..cd46467252a9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4475,7 +4475,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
return 0;
}
-static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
+static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
{
switch (arg) {
case KVM_CAP_USER_MEMORY:
@@ -5053,7 +5053,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
static long kvm_dev_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg)
{
- long r = -EINVAL;
+ int r = -EINVAL;
switch (ioctl) {
case KVM_GET_API_VERSION:
--
2.31.1
On 08/02/2023 14:01, Thomas Huth wrote:
> In case of success, this function returns the amount of handled bytes.
> However, this does not work for large values: The function is called
> from kvm_arch_vm_ioctl() (which still returns a long), which in turn
> is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that function
> stores the return value in an "int r" variable. So the upper 32-bits
> of the "long" return value are lost there.
>
> KVM ioctl functions should only return "int" values, so let's limit
> the amount of bytes that can be requested here to INT_MAX to avoid
> the problem with the truncated return value. We can then also change
> the return type of the function to "int" to make it clearer that it
> is not possible to return a "long" here.
>
> Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest")
> Signed-off-by: Thomas Huth <[email protected]>
Thanks for fixing this.
Reviewed-by: Steven Price <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 3 ++-
> arch/arm64/include/asm/kvm_host.h | 4 ++--
> arch/arm64/kvm/guest.c | 8 ++++++--
> 3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 0a67cb738013..f184427931fa 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5553,7 +5553,8 @@ with the KVM_XEN_VCPU_GET_ATTR ioctl.
> };
>
> Copies Memory Tagging Extension (MTE) tags to/from guest tag memory. The
> -``guest_ipa`` and ``length`` fields must be ``PAGE_SIZE`` aligned. The ``addr``
> +``guest_ipa`` and ``length`` fields must be ``PAGE_SIZE`` aligned.
> +``length`` must not be bigger than 2^31 - PAGE_SIZE bytes. The ``addr``
> field must point to a buffer which the tags will be copied to or from.
>
> ``flags`` specifies the direction of copy, either ``KVM_ARM_TAGS_TO_GUEST`` or
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 35a159d131b5..b1a16343767f 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> struct kvm_device_attr *attr);
>
> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> - struct kvm_arm_copy_mte_tags *copy_tags);
> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> + struct kvm_arm_copy_mte_tags *copy_tags);
>
> /* Guest/host FPSIMD coordination helpers */
> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index cf4c495a4321..cadef953046f 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> return ret;
> }
>
> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> - struct kvm_arm_copy_mte_tags *copy_tags)
> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> + struct kvm_arm_copy_mte_tags *copy_tags)
> {
> gpa_t guest_ipa = copy_tags->guest_ipa;
> size_t length = copy_tags->length;
> @@ -1035,6 +1035,10 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK)
> return -EINVAL;
>
> + /* Lengths above INT_MAX cannot be represented in the return value */
> + if (length > INT_MAX)
> + return -EINVAL;
> +
> gfn = gpa_to_gfn(guest_ipa);
>
> mutex_lock(&kvm->slots_lock);
On Wed, Feb 08, 2023, Thomas Huth wrote:
> The KVM_GET_NR_MMU_PAGES ioctl is quite questionable on 64-bit hosts
> since it fails to return the full 64 bits of the value that can be
> set with the corresponding KVM_SET_NR_MMU_PAGES call. Its "long" return
> value is truncated into an "int" in the kvm_arch_vm_ioctl() function.
>
> Since this ioctl also never has been used by userspace applications
> (QEMU, Google's internal VMM, kvmtool and CrosVM have been checked),
> it's likely the best if we remove this badly designed ioctl before
> anybody really tries to use it.
>
> Signed-off-by: Thomas Huth <[email protected]>
> ---
Reviewed-by: Sean Christopherson <[email protected]>
On Wed, Feb 08 2023, Thomas Huth <[email protected]> wrote:
> In case of success, this function returns the amount of handled bytes.
> However, this does not work for large values: The function is called
> from kvm_arch_vm_ioctl() (which still returns a long), which in turn
> is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that function
> stores the return value in an "int r" variable. So the upper 32-bits
> of the "long" return value are lost there.
>
> KVM ioctl functions should only return "int" values, so let's limit
> the amount of bytes that can be requested here to INT_MAX to avoid
> the problem with the truncated return value. We can then also change
> the return type of the function to "int" to make it clearer that it
> is not possible to return a "long" here.
>
> Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest")
> Signed-off-by: Thomas Huth <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 3 ++-
> arch/arm64/include/asm/kvm_host.h | 4 ++--
> arch/arm64/kvm/guest.c | 8 ++++++--
> 3 files changed, 10 insertions(+), 5 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On 2/9/23 1:01 AM, Thomas Huth wrote:
> In case of success, this function returns the amount of handled bytes.
> However, this does not work for large values: The function is called
> from kvm_arch_vm_ioctl() (which still returns a long), which in turn
> is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that function
> stores the return value in an "int r" variable. So the upper 32-bits
> of the "long" return value are lost there.
>
> KVM ioctl functions should only return "int" values, so let's limit
> the amount of bytes that can be requested here to INT_MAX to avoid
> the problem with the truncated return value. We can then also change
> the return type of the function to "int" to make it clearer that it
> is not possible to return a "long" here.
>
> Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest")
> Signed-off-by: Thomas Huth <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 3 ++-
> arch/arm64/include/asm/kvm_host.h | 4 ++--
> arch/arm64/kvm/guest.c | 8 ++++++--
> 3 files changed, 10 insertions(+), 5 deletions(-)
>
Reviewed-by: Gavin Shan <[email protected]>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 0a67cb738013..f184427931fa 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5553,7 +5553,8 @@ with the KVM_XEN_VCPU_GET_ATTR ioctl.
> };
>
> Copies Memory Tagging Extension (MTE) tags to/from guest tag memory. The
> -``guest_ipa`` and ``length`` fields must be ``PAGE_SIZE`` aligned. The ``addr``
> +``guest_ipa`` and ``length`` fields must be ``PAGE_SIZE`` aligned.
> +``length`` must not be bigger than 2^31 - PAGE_SIZE bytes. The ``addr``
> field must point to a buffer which the tags will be copied to or from.
>
> ``flags`` specifies the direction of copy, either ``KVM_ARM_TAGS_TO_GUEST`` or
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 35a159d131b5..b1a16343767f 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> struct kvm_device_attr *attr);
>
> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> - struct kvm_arm_copy_mte_tags *copy_tags);
> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> + struct kvm_arm_copy_mte_tags *copy_tags);
>
> /* Guest/host FPSIMD coordination helpers */
> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index cf4c495a4321..cadef953046f 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> return ret;
> }
>
> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> - struct kvm_arm_copy_mte_tags *copy_tags)
> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> + struct kvm_arm_copy_mte_tags *copy_tags)
> {
> gpa_t guest_ipa = copy_tags->guest_ipa;
> size_t length = copy_tags->length;
> @@ -1035,6 +1035,10 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK)
> return -EINVAL;
>
> + /* Lengths above INT_MAX cannot be represented in the return value */
> + if (length > INT_MAX)
> + return -EINVAL;
> +
> gfn = gpa_to_gfn(guest_ipa);
>
> mutex_lock(&kvm->slots_lock);
>
On 08/02/2023 15.01, Thomas Huth wrote:
> All kvm_arch_vm_ioctl() implementations now only deal with "int"
> types as return values, so we can change the return type of these
> functions to use "int" instead of "long".
>
> Signed-off-by: Thomas Huth <[email protected]>
> ---
> arch/arm64/kvm/arm.c | 3 +--
> arch/mips/kvm/mips.c | 4 ++--
> arch/powerpc/kvm/powerpc.c | 5 ++---
> arch/riscv/kvm/vm.c | 3 +--
> arch/s390/kvm/kvm-s390.c | 3 +--
> arch/x86/kvm/x86.c | 3 +--
> include/linux/kvm_host.h | 3 +--
> 7 files changed, 9 insertions(+), 15 deletions(-)
Ping!
Unless I missed something, I think this series had enough review ... Paolo,
could you maybe queue the whole series, since it's mostly an
architecture-wide clean up?
Thanks,
Thomas
On Wed, Feb 8, 2023 at 7:31 PM Thomas Huth <[email protected]> wrote:
>
> All kvm_arch_vm_ioctl() implementations now only deal with "int"
> types as return values, so we can change the return type of these
> functions to use "int" instead of "long".
>
> Signed-off-by: Thomas Huth <[email protected]>
Looks good to me.
For KVM RISC-V:
Acked-by: Anup Patel <[email protected]>
Regards,
Anup
> ---
> arch/arm64/kvm/arm.c | 3 +--
> arch/mips/kvm/mips.c | 4 ++--
> arch/powerpc/kvm/powerpc.c | 5 ++---
> arch/riscv/kvm/vm.c | 3 +--
> arch/s390/kvm/kvm-s390.c | 3 +--
> arch/x86/kvm/x86.c | 3 +--
> include/linux/kvm_host.h | 3 +--
> 7 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 9c5573bc4614..e791ad6137b8 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1449,8 +1449,7 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
> }
> }
>
> -long kvm_arch_vm_ioctl(struct file *filp,
> - unsigned int ioctl, unsigned long arg)
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> {
> struct kvm *kvm = filp->private_data;
> void __user *argp = (void __user *)arg;
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index a25e0b73ee70..84cadaa2c2d3 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -1003,9 +1003,9 @@ void kvm_arch_flush_remote_tlbs_memslot(struct kvm *kvm,
> kvm_flush_remote_tlbs(kvm);
> }
>
> -long kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> {
> - long r;
> + int r;
>
> switch (ioctl) {
> default:
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 04494a4fb37a..6f6ba55c224f 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -2386,12 +2386,11 @@ static int kvmppc_get_cpu_char(struct kvm_ppc_cpu_char *cp)
> }
> #endif
>
> -long kvm_arch_vm_ioctl(struct file *filp,
> - unsigned int ioctl, unsigned long arg)
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> {
> struct kvm *kvm __maybe_unused = filp->private_data;
> void __user *argp = (void __user *)arg;
> - long r;
> + int r;
>
> switch (ioctl) {
> case KVM_PPC_GET_PVINFO: {
> diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> index 65a964d7e70d..c13130ab459a 100644
> --- a/arch/riscv/kvm/vm.c
> +++ b/arch/riscv/kvm/vm.c
> @@ -87,8 +87,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> return r;
> }
>
> -long kvm_arch_vm_ioctl(struct file *filp,
> - unsigned int ioctl, unsigned long arg)
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> {
> return -EINVAL;
> }
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 8ad1972b8a73..86ca49814983 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2850,8 +2850,7 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> return r;
> }
>
> -long kvm_arch_vm_ioctl(struct file *filp,
> - unsigned int ioctl, unsigned long arg)
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> {
> struct kvm *kvm = filp->private_data;
> void __user *argp = (void __user *)arg;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2d210ab47e21..52a8c993cd55 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6645,8 +6645,7 @@ static int kvm_vm_ioctl_set_clock(struct kvm *kvm, void __user *argp)
> return 0;
> }
>
> -long kvm_arch_vm_ioctl(struct file *filp,
> - unsigned int ioctl, unsigned long arg)
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> {
> struct kvm *kvm = filp->private_data;
> void __user *argp = (void __user *)arg;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 4f26b244f6d0..ed2f1f02976b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1398,8 +1398,7 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
> bool line_status);
> int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> struct kvm_enable_cap *cap);
> -long kvm_arch_vm_ioctl(struct file *filp,
> - unsigned int ioctl, unsigned long arg);
> +int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg);
> long kvm_arch_vm_compat_ioctl(struct file *filp, unsigned int ioctl,
> unsigned long arg);
>
> --
> 2.31.1
>
>
> --
> kvm-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
Queued, thanks.
Paolo