| Message ID | ab7b9caf-5c90-4616-8517-b38637293639@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fix buffer overflow in kvm_arm_set_fw_reg() | expand |
On 19/04/2023 09:06, 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> Although there might be something to be said for rejecting anything where KVM_REG_SIZE(reg->id) != sizeof(u64), as for smaller sizes the top bits of val would be undefined which would require the code to mask the top bits out to be safe. Given that all registers are currently u64 (and I don't expect that to change), perhaps the below would be clearer? if (KVM_REG_SIZE(reg->id) != sizeof(val)) return -EINVAL; if (copy_from_user(&val, uaddr, sizeof(val))) return -EFAULT; Steve > --- > 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..4f5767fcaca5 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 -EINVAL; > if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id))) > return -EFAULT; >
Hi Dan, On 4/19/23 10:06, 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: Eric Auger <eric.auger@redhat.com> Thanks 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..4f5767fcaca5 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 -EINVAL; > if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id))) > return -EFAULT; >
On Wed, Apr 19, 2023 at 09:48:41AM +0100, Steven Price wrote: > On 19/04/2023 09:06, 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> > > Although there might be something to be said for rejecting anything > where KVM_REG_SIZE(reg->id) != sizeof(u64), as for smaller sizes the top > bits of val would be undefined which would require the code to mask the > top bits out to be safe. Given that all registers are currently u64 (and > I don't expect that to change), perhaps the below would be clearer? > > if (KVM_REG_SIZE(reg->id) != sizeof(val)) > return -EINVAL; > if (copy_from_user(&val, uaddr, sizeof(val))) > return -EFAULT; I was thinking that zero might be a valid size? regards, dan carpenter
On 19/04/2023 10:48, Dan Carpenter wrote: > On Wed, Apr 19, 2023 at 09:48:41AM +0100, Steven Price wrote: >> On 19/04/2023 09:06, 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> >> >> Although there might be something to be said for rejecting anything >> where KVM_REG_SIZE(reg->id) != sizeof(u64), as for smaller sizes the top >> bits of val would be undefined which would require the code to mask the >> top bits out to be safe. Given that all registers are currently u64 (and >> I don't expect that to change), perhaps the below would be clearer? >> >> if (KVM_REG_SIZE(reg->id) != sizeof(val)) >> return -EINVAL; >> if (copy_from_user(&val, uaddr, sizeof(val))) >> return -EFAULT; > > I was thinking that zero might be a valid size? Well any value of reg->id which doesn't match in the switch statement will cause a -ENOENT return currently, so a zero size would fail that way as it stands. So I don't think any size other than u64 is valid in the current code. There is obviously a question of return value - perhaps returning -ENOENT would be more appropriate if the size doesn't match (as a later kernel could decide to implement registers of different sizes)? Steve
On Wed, Apr 19, 2023 at 11:00:37AM +0100, Steven Price wrote: > On 19/04/2023 10:48, Dan Carpenter wrote: > > On Wed, Apr 19, 2023 at 09:48:41AM +0100, Steven Price wrote: > >> On 19/04/2023 09:06, 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> > >> > >> Although there might be something to be said for rejecting anything > >> where KVM_REG_SIZE(reg->id) != sizeof(u64), as for smaller sizes the top > >> bits of val would be undefined which would require the code to mask the > >> top bits out to be safe. Given that all registers are currently u64 (and > >> I don't expect that to change), perhaps the below would be clearer? > >> > >> if (KVM_REG_SIZE(reg->id) != sizeof(val)) > >> return -EINVAL; > >> if (copy_from_user(&val, uaddr, sizeof(val))) > >> return -EFAULT; > > > > I was thinking that zero might be a valid size? > > Well any value of reg->id which doesn't match in the switch statement > will cause a -ENOENT return currently, so a zero size would fail that > way as it stands. So I don't think any size other than u64 is valid in > the current code. > > There is obviously a question of return value - perhaps returning > -ENOENT would be more appropriate if the size doesn't match (as a later > kernel could decide to implement registers of different sizes)? Okay. I've sent a v2. Probably either -EINVAL or -ENOENT is fine, but -ENOENT is more helpful so let's return that. regards, dan carpenter
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index 2e16fc7b31bf..4f5767fcaca5 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 -EINVAL; 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> --- arch/arm64/kvm/hypercalls.c | 2 ++ 1 file changed, 2 insertions(+)