| Message ID | 20230208140105.655814-5-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Standardize on "int" return types instead of "long" | expand |
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 <thuth@redhat.com> Thanks for fixing this. Reviewed-by: Steven Price <steven.price@arm.com> > --- > 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 <thuth@redhat.com> 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 <thuth@redhat.com> > --- > 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 <cohuck@redhat.com>
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 <thuth@redhat.com> > --- > 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 <gshan@redhat.com> > 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); >
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);
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 <thuth@redhat.com> --- 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(-)