| Message ID | 4efbab8c-640f-43b2-8ac6-6d68e08280fe@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: arm64: Fix buffer overflow in kvm_arm_set_fw_reg() | expand |
On 19/04/2023 11:16, Dan Carpenter wrote: > The KVM_REG_SIZE() comes from the ioctl and it can be a power of two > between 0-32768 but if it is more than sizeof(long) this will corrupt > memory. > > Fixes: 99adb567632b ("KVM: arm/arm64: Add save/restore support for firmware workaround state") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Steven Price <steven.price@arm.com> Thanks, Steve > --- > v2: The original patch was okay but checking for != sizeof(val) is > stricter and more Obviously Correct[tm]. Return -ENOENT instead of > -EINVAL in case future ioctls are added which take a different size. > > arch/arm64/kvm/hypercalls.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index 2e16fc7b31bf..7fb4df0456de 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -544,6 +544,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > u64 val; > int wa_level; > > + if (KVM_REG_SIZE(reg->id) != sizeof(val)) > + return -ENOENT; > if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id))) > return -EFAULT; >
On 4/19/23 12:16, Dan Carpenter wrote: > The KVM_REG_SIZE() comes from the ioctl and it can be a power of two > between 0-32768 but if it is more than sizeof(long) this will corrupt > memory. > > Fixes: 99adb567632b ("KVM: arm/arm64: Add save/restore support for firmware workaround state") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: The original patch was okay but checking for != sizeof(val) is > stricter and more Obviously Correct[tm]. Return -ENOENT instead of > -EINVAL in case future ioctls are added which take a different size. Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > > arch/arm64/kvm/hypercalls.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index 2e16fc7b31bf..7fb4df0456de 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -544,6 +544,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > u64 val; > int wa_level; > > + if (KVM_REG_SIZE(reg->id) != sizeof(val)) > + return -ENOENT; > if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id))) > return -EFAULT; >
Hi Dan, On Wed, 19 Apr 2023 11:16:13 +0100, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The KVM_REG_SIZE() comes from the ioctl and it can be a power of two > between 0-32768 but if it is more than sizeof(long) this will corrupt > memory. > > Fixes: 99adb567632b ("KVM: arm/arm64: Add save/restore support for firmware workaround state") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: The original patch was okay but checking for != sizeof(val) is > stricter and more Obviously Correct[tm]. Return -ENOENT instead of > -EINVAL in case future ioctls are added which take a different size. > > arch/arm64/kvm/hypercalls.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index 2e16fc7b31bf..7fb4df0456de 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -544,6 +544,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > u64 val; > int wa_level; > > + if (KVM_REG_SIZE(reg->id) != sizeof(val)) > + return -ENOENT; > if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id))) > return -EFAULT; > Thanks for the fix. In the future, please Cc me on KVM/arm64 patches (specially this particular variety of patches...). Reviewed-by: Marc Zyngier <maz@kernel.org> M.
On Wed, Apr 19, 2023 at 12:31:53PM +0100, Marc Zyngier wrote:
> Thanks for the fix. In the future, please Cc me on KVM/arm64 patches
Sorry Marc, that wasn't intentional at all. I don't know what happened.
I have a script that I use send patches and it messed up both times. I
will investigate and fix.
regards,
dan carpenter
On Wed, 19 Apr 2023 13:16:13 +0300, Dan Carpenter wrote: > The KVM_REG_SIZE() comes from the ioctl and it can be a power of two > between 0-32768 but if it is more than sizeof(long) this will corrupt > memory. > > Applied to kvmarm/fixes, thanks! [1/1] KVM: arm64: Fix buffer overflow in kvm_arm_set_fw_reg() https://git.kernel.org/kvmarm/kvmarm/c/a25bc8486f9c -- Best, Oliver
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index 2e16fc7b31bf..7fb4df0456de 100644 --- a/arch/arm64/kvm/hypercalls.c +++ b/arch/arm64/kvm/hypercalls.c @@ -544,6 +544,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) u64 val; int wa_level; + if (KVM_REG_SIZE(reg->id) != sizeof(val)) + return -ENOENT; if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id))) return -EFAULT;
The KVM_REG_SIZE() comes from the ioctl and it can be a power of two between 0-32768 but if it is more than sizeof(long) this will corrupt memory. Fixes: 99adb567632b ("KVM: arm/arm64: Add save/restore support for firmware workaround state") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: The original patch was okay but checking for != sizeof(val) is stricter and more Obviously Correct[tm]. Return -ENOENT instead of -EINVAL in case future ioctls are added which take a different size. arch/arm64/kvm/hypercalls.c | 2 ++ 1 file changed, 2 insertions(+)